Skip to content

Fix --without-lsf and LSF in default search path #4204

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 3 commits into from
Sep 18, 2017

Conversation

jjhursey
Copy link
Member

 * Spaces not tabs, and indent properly
 * No functional changes here

Signed-off-by: Joshua Hursey <[email protected]>
 * Will display a message acknowledging the configure setting
   instead of 'simple ok' which is misleading.

Signed-off-by: Joshua Hursey <[email protected]>
…ach path

 * Reference Issue open-mpi#3546
 * If the user specified `--without-lsf` then do not check for it
   on the system, even if it is there. This can lead to the build
   failure identified in the issue above.

Signed-off-by: Joshua Hursey <[email protected]>
@jjhursey
Copy link
Member Author

To all that review this:

I did not try to rework the bulk of the logic in the lsf configure check on purpose. I'm trying for a minimal distance change that fixes the problem. The fix is similar to what @ggouaillardet proposed in the original ticket with some minor tweaks.
I did have to reformat a lot of the file since a good section of it had to be indented. I tried to break out those changes as best as I could.

Copy link
Contributor

@ggouaillardet ggouaillardet left a comment

Choose a reason for hiding this comment

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

i cannot test this since i do not have LSF installed. that being said, and minus my minor comment, that looks good to me.

AC_MSG_ERROR([Cannot continue])],
[AC_MSG_RESULT([sanity check ok ($2)])]
[AS_IF([test "x`ls $2/$3 2> /dev/null`" = "x"],
Copy link
Contributor

Choose a reason for hiding this comment

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

would test ! -e "$2/$3" do the trick here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possibly. I didn't want to change the prior behavior, so I left it as is. The git diff shows the indentation change in an odd way :/

Copy link
Member

Choose a reason for hiding this comment

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

Can't remember why we initially went with ls instead of test -e. Seems like a (slightly) odd choice, but ls works just fine.

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.

I can't test this, but it looks ok.

@rhc54
Copy link
Contributor

rhc54 commented Sep 13, 2017

@jjhursey Do we need to bring the change to opal_check_withdir over to PMIx?

@jjhursey
Copy link
Member Author

@rhc54 yeah we will. It's somewhat cosmetic, but does help clarify when looking at a snippet of configure output. I can do that once I merge this PR in and post the PRs to the various release branches - later this evening.

@jjhursey jjhursey merged commit 5cb5eb6 into open-mpi:master Sep 18, 2017
@jjhursey jjhursey deleted the fix/master/without-lsf branch September 18, 2017 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants