Fail if there's a bad library link #286
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
martin-g
reviewed
May 31, 2023
utils/build-universal.sh
Outdated
| for so_file in $arch_so_path/*.so; do | ||
| bad_shared_libraries=`otool -L "$so_file" | grep /opt/gfortran` | ||
| if [ ! -z "$bad_shared_libraries" ]; then | ||
| exit 1 |
There was a problem hiding this comment.
Would it be useful to print a warning/error mentioning the problematic .so file before exiting here ?
There was a problem hiding this comment.
The errors are displayed at 6e5bcac.
|
The logic of the nested is already implemented in |
|
Yes, that makes sense. I took it quite literally that the edit should be in |
|
Looks good but it would be nice to display an error message. I don't think many people will understand why we display a red ERROR label when the output of BUILD BIN doesn't show any error. Note that these ERRORs will always be something that we need to fix on our side (they reveal a misconfiguration of the builder) and not something that the developers will be able to address, so the error message should be clear about that in order to avoid confusion. Finally, if you've not done so already, it would be good to manually test the |
|
@hpages By an error message, is another way that it should display an error other than echoing a message? The error could read: "Error: Bad BBS configuration. Bad library paths not corrected in Mac binaries. Contact core team."? |
|
quick comment on "contact core team" -- some method should be provided ... maybe a URL to BBS github issues? |
|
|
|
I guess echoing the error message is the natural way to go. Whatever is echoed goes to the standard output which will be captured by BBS and displayed on the build report. Good idea to provide the details of how to contact us. bioc-devel is also a valid option. Additionally the script could send an email to [email protected] (using the same mechanism as |
|
I am leaving the link to issues so I can work on other planned issues. I know that we'll look at doing some notification in #297 anyway. I'll probably merge this on 9/12. I tested on lconway by cloning repos for the BBS, a4 (no .so file), and bandle (has .so file) and applying the proposed feature. Below are the results for bandle with good and bad library links for posterity: Good links Bad links (generated by commenting out |
|
Looks good. BBS calls Note that this exit code will end up on the report here https://bioconductor.org/checkResults/3.18/bioc-LATEST/bandle/lconway-buildbin.html, as the Finally note that including HTML in the output won't work. That's because the |
|
I've removed the html. Thanks for the pointer on checking the exit code. |
|
Great! I guess this is good to go then. Thanks! |
|
Thanks everyone for your input! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Am I headed in the right direction?
Close #282