-
Notifications
You must be signed in to change notification settings - Fork 53
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
Topic: pmi-simple - update to upstream (MPICH) state #1045
Conversation
Closing for now, tracking with issue #1046, and adding to the |
Requires some modifications for SOS: * include "config.h" instead of "mpichconf.h" * define MPI_MAX_PORT_NAME manually (w/out "mpi.h") * carry new pmi-simple reqs from MPICH (mpir_mem.h/mpl_sockaddr.h) * ignore strncasecmp/strnicmp requirement (not used in pmi-simple)
22baf90
to
ef7e754
Compare
This version of pmi-simple best aligns with pmodels/MPICH v3.2.1. Beyond that there were more significant changes that may not be practical to incorporate into SOS at the moment. However, this does eliminate all the compiler warnings from #1046 and passes CI, so I think now is possibly a good time to try it out for the v1.5.3 release. |
/* | ||
* Copyright (C) by Argonne National Laboratory | ||
* See COPYRIGHT in top-level directory | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just querying, do we have to change anything here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. The tests also do not show any more warnings. One question: the 3 new files - are they all verbatim to upstream or do we have some changes?
Question regarding "TODOs" left in the code. Should these be removed/resolved or remove/CR, or left as-is before upstream? |
@wrrobin - the tests are verbatim except for the 4 bullet items listed in the PR description, which are minimal but needed to be compatible with SOS.
@bcmIntc - since we keeping these changes "upstream" with respect to MPICH, we should keep as much as possible identical so we don't confuse ourselves or create unnecessary work if/when we update. However, if we fork off then we can remove/address these, but it's something we have avoided in the past for simplicity. |
This requires some slight modifications for SOS:
"config.h"
instead of"mpichconf.h"
MPI_MAX_PORT_NAME
manually (w/out including"mpi.h"
)mpir_mem.h
/mpl_sockaddr.h
)strncasecmp
/strnicmp
requirement (not used in pmi-simple)I'm not sure we want to make this change, but it does eliminate several warnings we see in our current version of pmi-simple. If we include it, we may want to defer this for a later SOS release (e.g., v1.5.2).