-
Notifications
You must be signed in to change notification settings - Fork 928
openmpi.spec: Add override install for openmpi-mca-params.conf #7677
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
Conversation
|
There's probably a better way to do this, don't have much experience writing spec files |
contrib/dist/linux/openmpi.spec
Outdated
| # type: bool (0/1) | ||
| %{!?install_params_file: %define install_params_file 0} | ||
| # type: string (full path of params file) | ||
| %{!?params_file: %define params_file openmpi-mca-params.conf} |
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.
Where does this default file come from?
Or is it an error to specify install_params_file==1 without also specifying a valid params_file?
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.
Yeah it's an error
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.
You should make it fail early, then. Otherwise, it doesn't fail until deep during make install, and the error isn't obvious -- it'll say openmpi-mca-params.conf not found, or something. And that will be uber confusing, because people will think that that file does exist.
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.
I was thinking instead to get rid of "install_params_file" and just have "params_file" and a check to see if the string isn't empty in the make install <-- just haven't had the time to test if I can do this
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.
@wckzhang Ping. Any progress, perchance?
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.
No sorry, I will put this on my TODO list
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.
I was having a few issues when testing so I put it off for later and never got back to it
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.
@wckzhang Do you think you'll finish this PR?
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.
I don't see it in the short term. If you want to close it, I can always re-open if I see the need in the future.
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.
You can close it if you like, or perhaps put it in draft mode.
|
Can one of the admins verify this patch? |
As the cuda copy code needs to be included into the datatype converter, it introduced an open-pal dependency on the common component. To avoid this, we move the code into util. This also addresses a bug with --enable-dynamic-dso and --with-cuda. Signed-off-by: William Zhang <[email protected]>
|
Crap, pushed to master branch caused this PR to be updated |
|
AWS isn't really interested in this PR at this point, so closing. |
Signed-off-by: William Zhang [email protected]