-
Notifications
You must be signed in to change notification settings - Fork 58
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
PS updated to 2.0.15 & SG updated to 4.0.22 #570
PS updated to 2.0.15 & SG updated to 4.0.22 #570
Conversation
SG updated to 4.0.22
Hey @CarlosHorro do you like to integrate my changes from here: #572? Or should be merge them separately? For this PR it seems that only an assertion needs to be adapted. |
Hi @bernt-matthias We may integrate them here, how do I do that?
|
Should be simply: |
1. remove stderr redirection in case of ``` command 2>> $temp_stderr && .... && cat $temp_stderr 2>&1; ``` stderr is swallowed if command fails. this was the case for PS, FC, and IP in case of ``` command 2>> $temp_stderr && .... ; cat $temp_stderr 2>&1; ``` the last cat is executed even if the command fails. the exit code of the command line is then the exit code of the cat, i.e. the tool will always be counted successful, even if everything fails. 2. fix peptideshaker find exec constructs Galaxy removes the last ';' from the generated command line. thus we need to make sure that the last `find ... -exec ... \;` is not the last command. 3. remove some unnecessay parentheses
@@ -12,8 +12,7 @@ | |||
</requirements> | |||
<expand macro="stdio" /> | |||
|
|||
<command><![CDATA[ | |||
## When supporting more advanced Galaxy versions: command use_shared_home="false" | |||
<command use_shared_home="false"><![CDATA[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be better to increase the profile
of the tools to something greater than or equal to 18.01.
I would suggest at least 20.01
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right @bernt-matthias, thank you. That was something I wanted to do some time ago but I didn't remember to include it...
Great, I think we have included enough stuff here :-) |
I think we should append |
Maybe like this: If you include this in the first line of the shell script:
|
Yep, it seems to work nicely, I will add it :-) |
One more thing: CarlosHorro#2 |
Interesting new tool errors... |
Well, the error seems related to trying to write into a system-related's folder: _ But I don't understand why this happens now and not just right after I included "command use_shared_home="false" in PS... |
Maybe because we did not see the exception until the trap has been included. You can simply try to revert the profile and set |
I will check if it is feasible to write that file in the temp folder. I will undo the profile otherwise... |
detect oom errors as fatal_oom and not fatal
I've seen that this is not immediate to solve. There is even another problem :
So I will undo the last changes until these issues are solved in the app itself. |
So, I guess we can merge this like it is and afterwards when PS app itself manages the paths in a better way we can create another PR. |
Agreed. Was just thinking if we can have Have you opened an issue at the PS repo? |
I still do not understand how the two error relate to the use of a shared home / a more recent profile. Or are the two exceptions non-critical? Then we could be more specific in the stdio regular expressions... |
Trying it...
Yes, just did it: |
Not sure either; however, 1) they don't look critical as PS has been working well without taking care of them, and 2) I think we need to move this forward and manage that all in the future PR. |
Agreed. But please also enable the newer profile for peptide_shaker. The profile is not the source of the problem, but the trap which appends the exception traces from the log files to stderr which is then caught by our stdio regular expressions, in particular
I already verified this locally. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will merge as soon as the CI passes
No description provided.