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

orte_data_server: fix a typo in ORTE_PMIX_PUBLISH_CMD handling #4167

Merged

Conversation

ggouaillardet
Copy link
Contributor

correctly balance some parenthesis ...

Fixes #4153

Thanks Austen Lauria for the report

This is a one-off commit for the v3.0.x branch, master was fixed as part of a larger commit,
and the v2 branches are unaffected.

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

correctly balance some parenthesis ...

Fixes open-mpi#4153

Thanks Austen Lauria for the report

This is a one-off commit for the v3.0.x branch, master was fixed as part of a larger commit,
and the v2 branches are unaffected.

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

@bwbarrett this fix is obvious (though finding the root cause was not ...) and though the faulty path is not always taken and does not always cause a crash, i made this PR a blocker so hopefully it can make it for v3.0.0.

Copy link
Contributor

@rhc54 rhc54 left a comment

Choose a reason for hiding this comment

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

Thanks!

@rhc54
Copy link
Contributor

rhc54 commented Sep 5, 2017

The failure is in the opal_path_nfs test, which has nothing whatever to do with this PR.

@gpaulsen
Copy link
Member

gpaulsen commented Sep 5, 2017

This is important to get into v3.0.0.

@bwbarrett
Copy link
Member

bot:ompi:retest

@bwbarrett
Copy link
Member

It looks like something changed on the Cray so that the nfs check now is consistently failing. I'm sure it doesn't have anything to do with this patch, but it needs to be addressed. @hppritcha, can you take a look? The test is only faling on the Cray and is consistently failing there.

@hppritcha
Copy link
Member

will do. We can ignore for this PR.

@hppritcha
Copy link
Member

actually there's nothing screwed up on the crays.

problem is that the entries in /proc/mounts on some cray systems can be enormous, much larger than the buffer being used in the get_mounts method in opal_patn_nfs.c. Anyway, one is suppose to use getmntent and friends to avoid this problem.

Do we want to fix the test or remove it from make check?

ggouaillardet added a commit to ggouaillardet/ompi that referenced this pull request Sep 6, 2017
ggouaillardet added a commit to ggouaillardet/ompi that referenced this pull request Sep 6, 2017
@jjhursey
Copy link
Member

jjhursey commented Sep 6, 2017

We should resolve the CI issues separate from this PR. This PR is an obvious, correct, critical fix. We should get it in the branch soon (today?) - it's absence is causing testing problems with optimized builds the longer we let it sit here.

@jsquyres
Copy link
Member

jsquyres commented Sep 6, 2017

@jjhursey #4179 was just merged that should fix the issue. Let's see if we can get a nice green checkmark now...

bot:ompi:retest

@rhc54
Copy link
Contributor

rhc54 commented Sep 6, 2017

Urrr...that commit went into master. How is that going to fix this PR into v3.0?

@bwbarrett
Copy link
Member

bwbarrett commented Sep 6, 2017

@jjhursey, the cranky RM for 3.0 (ie me) has said no green, no merge. So we need to get CI fixed...

@jsquyres
Copy link
Member

jsquyres commented Sep 6, 2017

@rhc54 Errr... right. Duh. v3.0.x PR filed here: #4180

PRs for other branches to be filed shortly.

@jjhursey
Copy link
Member

jjhursey commented Sep 6, 2017

Looks green to me 😄

@bwbarrett bwbarrett merged commit ba397f3 into open-mpi:v3.0.x Sep 6, 2017
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.

7 participants