Skip to content

configure: do look for sed #10400

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

Merged
merged 1 commit into from
May 18, 2022
Merged

Conversation

ggouaillardet
Copy link
Contributor

Refs. #10392

Signed-off-by: Gilles Gouaillardet [email protected]

@awlauria
Copy link
Contributor

Interesting, I'm not sure how this ever worked.

Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be getting invoked implicitly somehow in at least some cases. With AC 2.71 on my Mac, I see this in configure output (before this PR is applied):

checking for a sed that does not truncate output... /usr/bin/sed

Maybe that doesn't always happen in all versions of Autoconf...? 🤷‍♂️

Regardless, it does not hurt to explicitly invoke this to ensure that $SED is set.

@jsquyres
Copy link
Member

Answering my own question: I see what's happening. Yes, something is invoking this implicitly: $SED ultimately does get defined. But the error message that #10392 is referring to is apparently before $SED is defined (i.e., it's before the checking for a sed that does not truncate output... output that I cited). The error message occurs in some OMPI Fortran configury that probably doesn't have the same kind of machinery to ensure that $SED is defined. It's likely either an Autoconf bug or a bug in our Fortran configury.

Putting AC_PROG_SED at the top of configure.ac is a good solution.

@jsquyres
Copy link
Member

That being said, putting the AC_PROG_SED where it is right now means that configure output looks like this:

checking for a sed that does not truncate output... /usr/bin/sed

============================================================================
== Configuring Open MPI
============================================================================

*** Prerequisites
checking for perl... perl
...etc

While that's certainly correct, it would be 17.2% nicer if that sed test output was below the Configuring Open MPI banner. 😄

@awlauria
Copy link
Contributor

We could just move the opal_show_title "Configuring project_name_long" to be right before this line to get that 17.2%

Refs. open-mpi#10392

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet ggouaillardet force-pushed the topic/configury_sed branch from aee141f to 2012196 Compare May 18, 2022 07:38
@ggouaillardet
Copy link
Contributor Author

Agreed, sed is now a prerequisite and will be searched just before perl

BTW, how did you end up with 17.2%? Is there a reference I missed?

@jsquyres
Copy link
Member

BTW, how did you end up with 17.2%? Is there a reference I missed?

It was a complex set of calculations involving many mathematical disciplines, including linear algebra, calculus, hokus pokus, and imaginarium. Mostly the last one. Actually, entirely the last one. 😜

@jsquyres jsquyres merged commit a49e17f into open-mpi:main May 18, 2022
@awlauria
Copy link
Contributor

Should this also go to v4.0.x and v4.1.x?

@jsquyres
Copy link
Member

Should this also go to v4.0.x and v4.1.x?

I just checked: it does not appear to be necessary on v4.0.x or v4.1.x. 🎉

@awlauria
Copy link
Contributor

Interesting - is sed not used on those branches?

@jsquyres
Copy link
Member

I didn't track down why it isn't necessary. But I checked both branches, and the same erroneous output (indicating that $SED is not set) does not appear in the configure output. I did this testing on my laptop, where the same GNU Autotools were used to replicate the bad $SED-is-not-set behavior on main/v5.0.x.

Something must be different on the v4.x branches -- either the Fortran tests don't use $SED or some dependency ordering is different than sed is searched for earlier, ... etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants