Skip to content
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

configury: revamp ucx detection #4383

Merged
merged 1 commit into from
Oct 25, 2017

Conversation

ggouaillardet
Copy link
Contributor

  • when --with-ucx=DIR is not set, try the default path and fallback to /opt/ucx
  • when --with-ucx-libdir is not set, try lib64 and then lib directories
  • do not handle --with-ucx-libdir (this is a user mistake, no need to over-complicate our logic)

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

 - when --with-ucx=DIR is not set, try the default path and fallback to /opt/ucx
 - when --with-ucx-libdir is not set, try lib64 and then lib directories
 - do not handle --with-ucx-libdir (this is a user mistake, no need to over-complicate our logic)

Signed-off-by: Gilles Gouaillardet <[email protected]>
@ggouaillardet
Copy link
Contributor Author

:bot:aws:retest

1 similar comment
@ggouaillardet
Copy link
Contributor Author

:bot:aws:retest

@bwbarrett
Copy link
Member

bot:ompi:retest

@ggouaillardet, the OMPI Jenkins sitting in AWS didn't understand bot:aws:retest (only ompi: or lanl:). It seemed like a reasonable guess, so I added aws: to the list as well.

@ggouaillardet
Copy link
Contributor Author

Thanks !
so that means there is a bigger mystery here. i issued the aws command on #4384 and it instantly triggered a rebuilt ! then i issued the same aws command twice on this PR, but nothing happened ... and still nothing happened with the ompi command you issued.

@bwbarrett
Copy link
Member

there was a typo in my update to make bot:aws:retest work. It never would have worked before (but there are some other updates, like a new commit, that can cause a retest.

bot:aws:retest

@ggouaillardet
Copy link
Contributor Author

thanks !

@ggouaillardet ggouaillardet merged commit c4650b5 into open-mpi:master Oct 25, 2017
@jsquyres
Copy link
Member

I'm confused by the commit message on this commit:

 - when --with-ucx-libdir is not set, try lib64 and then lib directories
 - do not handle --with-ucx-libdir (this is a user mistake, no need to over-complicate our logic)

The 1st line says it will handle --with-ucx-libdir. But the 2nd line says it will not handle --with-ucx-libdir. Which is it?

@ggouaillardet
Copy link
Contributor Author

configure --with-ucx=/foo will look for lib in /foo/lib64 and /foo/lib
configure --with-ucx=/foo --with-ucx-libdir=/foo/bar will look for lib in /foo/bar
configure --with-ucx=/foo --with-ucx-libdir will look for lib in yes (this is a dumb thing to do, and we do not protect end users against themselves any more in this case)

@jsquyres
Copy link
Member

Ooohhhh... I see. I'm sorry to be type-A, but can you add something to that 3rd line in the commit to explain the yes value?

...and/or, what do we do elsewhere if the user specifies --with-FOO-libdir (with no value) -- do we ignore, or do we abort? We should be consistent.

@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Oct 26, 2017

ok, i ll reword this.
I checked here and there, and in other places, if --with-FOO-libdir is used, then the libdir is yes (e.g. we do nothing special), i will check again to be sure.

@jsquyres
Copy link
Member

Is it worth making a support m4 macro that we can use in multiple places (for --with-FOO and --with-FOO-libdir handling) to guarantee consistent handling of these CLI args across the code base?

@ggouaillardet
Copy link
Contributor Author

ggouaillardet commented Oct 27, 2017

it turns we treat --with-FOO-libdir=yes as a no-op almost everywhere. (and a notable exception is opal_check_pmi.m4 and this is what i used as a reference ...)
fwiw, if we gently ignore --with-FOO-libdir, should we also ignore --without-FOO-libdir
and yes, a macro could be helpful to achieve consistency

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

Successfully merging this pull request may close these issues.

3 participants