Skip to content

Conversation

@rhc54
Copy link
Contributor

@rhc54 rhc54 commented Aug 3, 2017

Fixes #3980
Closes #4007
Refs #3985

Signed-off-by: Ralph Castain rhc@open-mpi.org

Copy link
Member

@jjhursey jjhursey left a comment

Choose a reason for hiding this comment

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

This looks great. I'd wait for the other review before merging, but I think this does what we outlined. Thanks!

opal_unsetenv(OPAL_MCA_PREFIX"pmix", &orte_launch_environ);
/* clear any install_prefix that might have been set so
* we don't cause our children to falsely generate warnings */
opal_unsetenv("PMIX_INSTALL_PREFIX", &orte_launch_environ);
Copy link
Member

Choose a reason for hiding this comment

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

Question:
Do we want to clear PMIX_INSTALL_PREFIX here? What if it was set in the user's environment to point to the correct external PMIx install (assume they are also configured Open MPI with an external PMIx). Wouldn't the children need this value to be set when they are launched as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's fine - it was being cleared to avoid conflicts on the internal version if OPAL_PREFIX was set and we then set PMIX_INSTALL_PREFIX in the daemon, thus delivering both to the child.

…hes between location directives for OPAL and PMIx. Provide a more helpful error message and error out if we find a mismatch. If any OPAL values are set and the PMIx equivalent is not, then transfer it.

Do not clear PMIX_INSTALL_PREFIX from the daemon's launch environment

Fixes #3980
Closes #4007
Refs #3985

Signed-off-by: Ralph Castain <rhc@open-mpi.org>
@rhc54
Copy link
Contributor Author

rhc54 commented Aug 7, 2017

@artpol84 This look okay to you too?

Copy link
Contributor

@artpol84 artpol84 left a comment

Choose a reason for hiding this comment

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

👍

}
OPAL_LIST_DESTRUCT(&values);
return OPAL_SUCCESS;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd probably do else here

Copy link
Contributor

Choose a reason for hiding this comment

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

ohh, it's a return - never mind then!

@rhc54 rhc54 merged commit 9921237 into open-mpi:master Aug 7, 2017
@rhc54 rhc54 deleted the topic/p3 branch August 7, 2017 18:42
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