-
Notifications
You must be signed in to change notification settings - Fork 705
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 regular configure
instead of wrapper script for recent UCX easyconfigs
#20428
use regular configure
instead of wrapper script for recent UCX easyconfigs
#20428
Conversation
@boegelbot please test @ generoso |
@bedroge: Request for testing this PR well received on login1 PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2079607477 processed Message to humans: this is just bookkeeping information for me, |
@boegelbot please test @ jsc-zen3 |
configure
wrapper script for recent UCX easyconfigsconfigure
instead of wrapper script for recent UCX easyconfigs
@bedroge: Request for testing this PR well received on jsczen3l1.int.jsc-zen3.fz-juelich.de PR test command '
Test results coming soon (I hope)... - notification for comment with ID 2079612506 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
Test report by @boegelbot |
We also use |
Ah, good catch, didn't know it was used in other places. I suppose it makes sense to use the same approach everywhere, so I can make a PR for that easyblock as well. However, @SebastianAchilles found out that this patch is actually not really required anymore to solve the RISC-V issue (since the edit: and it may still be useful for UCX 1.16.0. Even though that version does support RISC-V out of the box (and hence doesn't need the patch from #20429), they still ship a very old |
I've applied the same change to the UCX plugins easyblock in easybuilders/easybuild-easyblocks#3315. I've also opened a PR for UCX 1.16.0 (#20431), which demonstrates that this change is also still useful for RISC-V builds. |
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.
this approach may still be a bit cleaner anyway
Yes, I share your opinion, using the regular configure
seems a bit cleaner.
I've applied the same change to the UCX plugins easyblock in easybuilders/easybuild-easyblocks#3315. I've also opened a PR for UCX 1.16.0 (#20431), which demonstrates that this change is also still useful for RISC-V builds.
Since this change will be useful for future UCX easyconfigs for RISC-V I think it makes sense to merge this PR.
@branfosj Do you agree?
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.
make sense to circumvent the contrib/configure-release
script, since it doesn't add much value, and causes trouble since updated config.guess
is not picked up
Test report by @boegel |
Going in, thanks @bedroge! |
The UCX easyconfigs for version 1.9.0 and newer are currently using
configure_cmd = contrib/configure-release
; this is a wrapper script around./configure
which passed some predefined flags (these haven't changed for many years), see:https://github.com/openucx/ucx/blob/master/contrib/configure-release
While trying to install UCX on RISC-V, I ran into issues with the outdated
config.guess
included with UCX. The ConfigureMake easyblock should download a newer version and use that one, but it turns out that it only does that when theconfigure_cmd
script includes a certain string (Generated by GNU Autoconf
). Though theconfigure
script does include it, this is not the case for thecontrib/configure-release
wrapper script, hence that step is skipped. By appending the options that this wrapper uses toconfigopts
, we can just call./configure
itself, and then EB does make sure that an updatedconfig.guess
is being used. Additionally, it's more explicit in the easyconfig which flags are being passed.