-
Notifications
You must be signed in to change notification settings - Fork 704
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
speed up OpenMPI 4.1.4 configure by not running "autogen.pl --force", but only running required Autotools commands #15957
speed up OpenMPI 4.1.4 configure by not running "autogen.pl --force", but only running required Autotools commands #15957
Conversation
Test report by @Flamefire |
Add `cd ..` as its own command like the others
@boegelbot please test @ generoso |
@boegel: Request for testing this PR well received on login1 PR test command '
Test results coming soon (I hope)... - notification for comment with ID 1204033126 processed Message to humans: this is just bookkeeping information for me, |
Test report by @boegelbot |
@@ -36,7 +36,16 @@ dependencies = [ | |||
('UCC', '1.0.0'), | |||
] | |||
|
|||
preconfigopts = './autogen.pl --force && ' | |||
preconfigopts = ' && '.join([ |
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.
@Flamefire @bartoldeman I think it makes sense to add a comment block above this line to explain i) why this is needed, ii) why we're not using autogen.pl
, iii) why the commands we are using are sufficient.
Without that, people are bound to start wondering in the future why we're doing it like this rather than just running autogen.pl
...
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.
Just FYI I did a diff between the result for both methods and the only differences are updates to various autoconf-supplied files which are irrelevant to the changes in the patches. So it's safe.
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.
Added 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
@boegelbot Please test @ jsc-zen2 |
@akesandgren: Request for testing this PR well received on jsczen2l1.int.jsc-zen2.easybuild-test.cluster PR test command '
Test results coming soon (I hope)... - notification for comment with ID 1230357738 processed Message to humans: this is just bookkeeping information for me, |
Requested change implemented
Test report by @boegelbot |
Going in, thanks @Flamefire! |
(created using
eb --new-pr
)This runs only a (relevant) subset of the
autogen.pl
script as suggested by @Micket on Slack which seemingly speeds up the configure enough to make it not timeout on some machines (e.g. our PPC cluster)