-
Notifications
You must be signed in to change notification settings - Fork 307
use custom RPATH sanity check for Go packages that doesn't actually check for an RPATH section in the binary #2913
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
use custom RPATH sanity check for Go packages that doesn't actually check for an RPATH section in the binary #2913
Conversation
|
Test report by @bedroge Overview of tested easyconfigs (in order)
Build succeeded for 1 out of 1 (1 easyconfigs in total) |
|
Test report by @bedroge Overview of tested easyconfigs (in order)
Build succeeded for 4 out of 4 (4 easyconfigs in total) Edit: don't know what went wrong with that first link, but it's |
boegel
left a comment
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.
@bedroge Rather than copy-pasting the body of EasyBlock.sanity_check_rpath and stripping out the stuff related to readelf_rpath_regex, can't we make the readelf part of EasyBlock.sanity_check_rpath optional (but enabled by default), so we can just do this here?
super(EB_Go, self).sanity_check_rpath(check_readelf_rpath=False)
Yes, that's a good idea, makes it much more reusable. Related to that: the issue also popped up for easybuilders/easybuild-easyconfigs#17516, but this makes me wonder if there's a way to easily apply the same thing for easyconfigs like that one without having to implement a custom easyblock? Would it, for instance, make sense to have a list of application names for which the step gets skipped anyway, or add some easyconfig parameter that can be used to skip the step? |
We could indeed also add an easyconfig parameter so the |
|
Implemented the first idea in https://github.com/easybuilders/easybuild-framework/pull/4249/files, the latter can be done in an additional PR. |
|
Test report by @bedroge Overview of tested easyconfigs (in order)
Build succeeded for 5 out of 5 (5 easyconfigs in total) Note: used a development version of |
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) edit: using |
|
Test report by @boegel Overview of tested easyconfigs (in order)
Build succeeded for 3 out of 3 (3 easyconfigs in total) edit: without |
boegel
left a comment
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.
lgtm
With RPATH support enabled, the RPATH sanity check of the
GoPackageeasyblock always fails. See easybuilders/easybuild-easyconfigs#17516.This implements @casparvl's suggestion (easybuilders/easybuild-easyconfigs#17516 (comment)) and doesn't run the
readelfcommand on these binaries. In the rare case that a binary does actually depend on some external library, an error should be printed that explains that the current RPATH wrappers don't work for Go, and a link to the issue is printed as well.