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

Linting fix for W605 #888

Merged
merged 2 commits into from Nov 8, 2018

Conversation

Projects
None yet
5 participants
@martenson
Copy link
Member

martenson commented Nov 7, 2018

No description provided.

martenson added some commits Nov 7, 2018

@peterjc

This comment has been minimized.

Copy link
Contributor

peterjc commented Nov 8, 2018

Looks good to me - does the Python 3.6 failure on TravisCI look like a glitch?

@erasche

This comment has been minimized.

Copy link
Member

erasche commented Nov 8, 2018

@bebatut you can probably use the same thing in #887

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Nov 8, 2018

Yes and no, I'm assuming this is because you get some old python version if you don't specify python 2 in the build matrix. The warnings are printed to stderr, and that makes the set metadata tool fail.

@mvdbeek mvdbeek merged commit b334501 into galaxyproject:master Nov 8, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Nov 8, 2018

Looks good to me - does the Python 3.6 failure on TravisCI look like a glitch?

It is a consequence of pyca/cryptography#4261 , introduced in cryptography 2.3.0 . We use 2.3.1 since Galaxy release 18.09 . Ubuntu Trusty (used in Travis) still uses python 2.7.5, which causes the warning to be printed on stderr by cryptography, making some tool fail.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Nov 8, 2018

This is a Galaxy issue though, do we want to force deployers to upgrade their python version, or do we want to ignore stderr messages in the set_metadata tool ?

@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Nov 8, 2018

@mvdbeek Something like the following?

diff --git a/lib/galaxy/datatypes/set_metadata_tool.xml b/lib/galaxy/datatypes/set_metadata_tool.xml
index 69c84dd..3c52a72 100644
--- a/lib/galaxy/datatypes/set_metadata_tool.xml
+++ b/lib/galaxy/datatypes/set_metadata_tool.xml
@@ -4,7 +4,7 @@
         <requirement type="package" version="1.5">bcftools</requirement>
     </requirements>
     <action module="galaxy.tools.actions.metadata" class="SetMetadataToolAction"/>
-    <command>"\${GALAXY_PYTHON:-python}" "${set_metadata}" ${__SET_EXTERNAL_METADATA_COMMAND_LINE__}</command>
+    <command detect_errors="exit_code">"\${GALAXY_PYTHON:-python}" "${set_metadata}" ${__SET_EXTERNAL_METADATA_COMMAND_LINE__}</command>
     <configfiles>
         <configfile name="set_metadata">from galaxy_ext.metadata.set_metadata import set_metadata; set_metadata()</configfile>
     </configfiles>
@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Nov 8, 2018

Yes. But this will also bite deployers on all tools that require galaxy's virtualenv and that determine the state based on the presence of messages on stderr.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Nov 8, 2018

But this is a good change, I think, we should have this regardless.

@nsoranzo

This comment has been minimized.

Copy link
Member

nsoranzo commented Nov 8, 2018

I'm not much convinced it would fix anything though, since the set_metadata part of the galaxy_JOBID.sh job script would still print the message on the job stderr, causing the tool to fail.

@mvdbeek

This comment has been minimized.

Copy link
Member

mvdbeek commented Nov 8, 2018

Right, but it seems more appropriate to work with exit codes anyway. The right thing from the deployer side is certainly upgrading python, those warnings aren't there for no good reason.

@martenson martenson deleted the martenson:linting-fix branch Nov 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment