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

v2.x: configury: do not add -I/usr/include -L/usr/lib[64] when UCX is insta… #4399

Merged
merged 1 commit into from
Dec 19, 2017

Conversation

ggouaillardet
Copy link
Contributor

…lled in /usr

Fixes #4345

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

(back-ported from commit af03f55)

…lled in /usr

Fixes open-mpi#4345

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

(back-ported from commit open-mpi/ompi@af03f55)
@jsquyres
Copy link
Member

bot:ompi:retest

@hppritcha
Copy link
Member

@jladd-mlnx please review.

@hppritcha hppritcha requested review from artpol84 and removed request for jladd-mlnx December 7, 2017 16:44
@hppritcha hppritcha assigned artpol84 and unassigned jladd-mlnx Dec 7, 2017
@hppritcha hppritcha requested review from karasevb and removed request for artpol84 December 7, 2017 16:45
@hppritcha hppritcha assigned karasevb and unassigned artpol84 Dec 7, 2017
@ggouaillardet
Copy link
Contributor Author

:bot:aws:retest

@ggouaillardet
Copy link
Contributor Author

@jladd-mlnx @karasevb could you guys review this ? currently, jenkins fail to build PR on ARMv8 and the v2.x branch (for example, see https://jenkins.open-mpi.org/jenkins/job/open-mpi.build.platforms/1889/Platform=ARMv8/), and i think this PR will fix that. I just triggered a rebuild in order to confirm this.

@artpol84
Copy link
Contributor

artpol84 commented Dec 8, 2017

@karasevb will check till next week
Thank you for fixing that!

Copy link
Member

@karasevb karasevb left a comment

Choose a reason for hiding this comment

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

👍

@@ -64,7 +63,8 @@ AC_DEFUN([OMPI_CHECK_UCX],[
AC_MSG_CHECKING(for UCX version compatibility)
AC_REQUIRE_CPP
old_CPPFLAGS="$CPPFLAGS"
CPPFLAGS="$CPPFLAGS -I$ompi_check_ucx_dir/include"
AS_IF([test -n "$ompi_check_ucx_dir"],
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't you check for $ompi_check_ucx_dir/include?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that's the same test; this is a test to see if the string has a value, not that the directory exists...

@hppritcha
Copy link
Member

We need this PR merged in to fix thunderx1 jenkins problems.
@shamisp

Copy link
Member

@bwbarrett bwbarrett left a comment

Choose a reason for hiding this comment

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

Can you shorten / clean up your commit message title? Reminder from the git manual:

Though not required, it’s a good idea to begin the commit message with a single short (less than 50 character) line summarizing the change, followed by a blank line and then a more thorough description. The text up to the first blank line in a commit message is treated as the commit title, and that title is used throughout Git. For example, git-format-patch[1] turns a commit into email, and it uses the title on the Subject line and the rest of the commit in the body.

@rhc54
Copy link
Contributor

rhc54 commented Dec 12, 2017

Huh - nice to know. I'd never heard of that "guidance"

@bwbarrett
Copy link
Member

@rhc54, I'm not sure if you're being serious, but yeah, it's pretty common practice with git repos. It makes git log --oneline way more readable. Many of the Linux-oriented open source projects have way stricter rules than even that suggestion, around information that should be in subject vs. body, but one step at a time :).

@bwbarrett bwbarrett dismissed their stale review December 19, 2017 16:07

not a big deal

@hppritcha
Copy link
Member

@bwbarrett is okay with the messy commit message since its not going in to master

@hppritcha hppritcha merged commit d793f08 into open-mpi:v2.x Dec 19, 2017
@jsquyres jsquyres changed the title configury: do not add -I/usr/include -L/usr/lib[64] when UCX is insta… v2.x: configury: do not add -I/usr/include -L/usr/lib[64] when UCX is insta… Jan 5, 2018
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.

8 participants