Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: R 3 2 2 and cairo #312

Closed
wants to merge 8 commits into from

Conversation

mvdbeek
Copy link
Member

@mvdbeek mvdbeek commented Oct 9, 2015

As discussed in issue #304, here is a new version of R, package_cairo and package_r_cairo, as well as some changes in the exposed include and lib paths to get this working.

Conflicts:
	packages/package_cairo_1_14_2/.shed.yml
	packages/package_cairo_1_14_2/tool_dependencies.xml
@@ -31,7 +31,7 @@
</repository>
</action>
<action type="shell_command">
export LDFLAGS="-Wl,-rpath,$PIXMAN_LIB_PATH -Wl,-rpath,$LIBPNG_ROOT/lib -Wl,-rpath,$FREETYPE_LIB_PATH" &amp;&amp; \
export LDFLAGS="-Wl,-rpath,$PIXMAN_LIB_PATH -Wl,-rpath,$LIBPNG_ROOT/lib -Wl,-rpath,$FREETYPE_LIB_PATH" &amp;&amp; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\ at the end should be removed.

@yhoogstrate
Copy link
Member

http://downloads.sourceforge.net/project/libpng/libpng16/older-releases/1.6.7/libpng-1.6.7.tar.gz
is can be downloaded via https

edit: WHOOPS not! wget forwarded me !

@@ -5,7 +5,7 @@
<actions>
<action type="download_by_url">http://downloads.sourceforge.net/project/libpng/libpng16/older-releases/1.6.7/libpng-1.6.7.tar.gz</action>
<action type="shell_command">./configure --prefix=$INSTALL_DIR</action>
<action type="shell_command">make &amp;&amp; make install</action>
<action type="make_install" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can reduce it even further:
<action type="autoconf">--prefix=$INSTALL_DIR</action>

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 9, 2015

@yhoogstrate OK, I guess I should also add md5 sums?

@bgruening
Copy link
Member

Let's wait with merging until we have the binaries.

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 9, 2015

@yhoogstrate I don't think that forwarding would be a problem for urllib2, which is used for downloading. Also we got the shasum. I'll go ahead with https?

…ine continuation for cairo. Adjust binary R download link.
@davebx
Copy link
Contributor

davebx commented Oct 9, 2015

@yhoogstrate
Copy link
Member

The URL gets redirected to HTTP because HTTPS seems not to work. So I would not go for HTTPS url. Forwarding to HTTP is really not something you want, because that may introduce MITM attacks.

SHA256SUMS are really good because they confirm whether the transfer completed correctly, source code hasn't been modified over time and no MITM attack has taken place.

@bgruening
Copy link
Member

super fast @davebx!

@@ -31,7 +31,7 @@
</repository>
</action>
<action type="shell_command">
export LDFLAGS="-Wl,-rpath,$PIXMAN_LIB_PATH -Wl,-rpath,$LIBPNG_ROOT/lib -Wl,-rpath,$FREETYPE_LIB_PATH" &amp;&amp; \
export LDFLAGS="-Wl,-rpath,$PIXMAN_LIB_PATH -Wl,-rpath,$LIBPNG_ROOT/lib -Wl,-rpath,$FREETYPE_LIB_PATH"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do need the && and the end isn't it? We could also use CDATA here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CDATA definitely makes sense, the && is not needed, but can be added back. It depends if you prefer one or 2 issued commands, If you prefer one command I'll also need to add back the line continuation character \.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you like :) But this doesn't seem to work currently.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My recommendation would be && and no \

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, no problem. Still stuck with #312 (comment) though.

@mvdbeek mvdbeek changed the title R 3 2 2 and cairo WIP: R 3 2 2 and cairo Oct 11, 2015
@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 11, 2015

@bgruening would you mind taking a look at this pixman tool_dependencies.xml?
https://gist.github.com/mvdbeek/9fdf008c010435087f4e
It is now not receiving the environmental variables defined by libpng ...
I have been scratching my head for a while on this one now

@bgruening
Copy link
Member

It this somewhere in the TS. No obvious mistake that I can see.

@bgruening
Copy link
Member

@mvdbeek can you updload it in some test account in the TS. I will have a look at this in the evening. Just need a clean docker instance.

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 12, 2015

@bgruening
This one is from the gist, and doesn't receive the LIBPNG_LIB_PATH:
https://testtoolshed.g2.bx.psu.edu/view/mvdbeek/package_pixman_0_32_6_actions_group_test/e7dbececd3d0
This one has the uname stuff to get the OS, and correctly recevices LIBPNG_LIB_PATH:
https://testtoolshed.g2.bx.psu.edu/view/mvdbeek/package_pixman_0_32_6_osx_bash/461e0b3b76b4
Don't be fooled if both install correctly. When you check the INSTALL.log, you should see

PATH=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6/bin:$PATH; export PATH
LIBPNG_ROOT=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6; export LIBPNG_ROOT
LIBPNG_LIB_PATH=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6/lib; export LIBPNG_LIB_PATH
LIBPNG_INCLUDE_PATH=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6/include; export LIBPNG_INCLUDE_PATH
LD_LIBRARY_PATH=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6/lib:$LD_LIBRARY_PATH; export LD_LIBRARY_PATH
PKG_CONFIG_PATH=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6/lib/pkgconfig:$PKG_CONFIG_PATH; export PKG_CONFIG_PATH
LIBPNG_ROOT_DIR=/home/marius/src/galaxy/tool-dependencies/libpng/1.6.7/iuc/package_libpng_1_6_7/ac6ef08bf0e6; export LIBPNG_ROOT_DIR
...

This doesn't happen with the first repo.

@yhoogstrate
Copy link
Member

@mvdbeek In the examples I've seen so far <action type="set_environment"> is not within a <actions ...> element. Is it possible this has something to do with it?

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 12, 2015

I don't think so (see https://github.com/galaxyproject/tools-iuc/blob/master/packages/package_r_3_2_1/tool_dependencies.xml).
Anyway, the problematic action is <action type="set_environment_for_install">, which populates the environmental variables during the install process.
It looks like <action type="set_environment_for_install"> doesn't work within actions_group tags to me

@bgruening
Copy link
Member

@mvdbeek Oh I see! How did you updated the repository. The toolshed and the revisions are missing. This should not happen and is a bug if you uploaded it with planemo.

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 12, 2015

I did upload with planemo ... you think it's a bug in the toolshed, right?
That does explain the things. i never bothered to actually check in the toolshed :D

@bgruening
Copy link
Member

@mvdbeek actually I thought this was fixed in galaxyproject/galaxy#637
This makes me wonder how old the TTS is :(

@martenson
Copy link
Member

I updated the TTS
you can see how old is the deployed TTS version here: https://github.com/galaxyproject/usegalaxy-playbook/commits/master/stage/group_vars/toolshedservers.yml

@bgruening
Copy link
Member

Thanks @martenson. Hopefully this fixes @mvdbeek issue.

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 13, 2015

It does, thx @martenson @bgruening

@mvdbeek
Copy link
Member Author

mvdbeek commented Oct 16, 2015

I'm closing this PR in favor of more incremental changes:

@mvdbeek mvdbeek closed this Oct 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants