-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
openmpi: add two_level_namespace variant for MacOS #47202
base: develop
Are you sure you want to change the base?
openmpi: add two_level_namespace variant for MacOS #47202
Conversation
…able building executables and libraries with two level namespace enabled.
# patch needs to be applied (again) AFTER autoreconf ran. | ||
def patch(self): | ||
spec = self.spec | ||
if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"): |
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.
if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"):
->
if spec.satisfies("+two_level_namespace platform=darwin"):
def patch(self): | ||
spec = self.spec | ||
if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"): | ||
print("Applying configure patch for two_level_namespace on MacOS") |
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.
Is this necessary? I don't think most custom patch()
functions print anything.
@@ -996,6 +1016,9 @@ def autoreconf(self, spec, prefix): | |||
def autoreconf(self, spec, prefix): | |||
perl = which("perl") | |||
perl("autogen.pl", "--force") | |||
if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"): | |||
print("Re-applying configure patch for two_level_namespace on MacOS after autoreconf") |
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.
Same question as above-- do we need to print a message here?
@@ -996,6 +1016,9 @@ def autoreconf(self, spec, prefix): | |||
def autoreconf(self, spec, prefix): | |||
perl = which("perl") | |||
perl("autogen.pl", "--force") | |||
if "+two_level_namespace" in spec and spec.satisfies("platform=darwin"): |
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.
Recommend same change in condition here.
@AlexanderRichert-NOAA thanks for the quick review! I've addressed your comments in the latest commit. |
Does this PR work in spack develop? I am having trouble with it. I had to move a few things around for the concretizer to accept it:
|
@climbfuji thanks for the feedback. These changes haven't been tested with the JCSDA development branch until now. I initially tried to cherry-pick the commits from the JCSDA fork to create this PR, but there were many changes unrelated to the two-level namespace that got pulled in. It wasn't clear how to resolve these, so I took the path of manually updating this feature branch by diff'ing the authoritative spack develop branch with the JCSDA spack development branch and making only the changes related to the two-level namespace processing. I was relying on the CI testing in this PR and careful checking of the update to this feature branch to get this PR right, but apparently this didn't work. My apologies if this has caused any inconvenience. In retrospect, I think the proper approach would have been to wait until the next merge of the authoritative spack develop branch into the JCSDA fork was completed and then create a PR from that point with the two-level namespace variant. I think it would be good to take this approach now, unless others object. We could start with the changes @climbfuji needed to get this working after he merged the authoritative spack develop branch. @climbfuji has a draft PR (JCSDA#482) in the works so once that PR gets merged, I can redo this PR. If this seems agreeable with everyone, then I'll close this PR and create a new one after JCSDA#482 gets merged into the JCSDA fork. |
The code I pasted in my above comment works with spack develop, you can just use it as is for your PR instead of what you have (it is from my effort of pulling in spack develop into our fork). |
…ariant to get concretize to work properly.
@climbfuji thanks for the advice and code changes! I've updated with your version of the openmpi package.py script that you posted above. I removed a block of code that was commented out in the posted copy. @AlexanderRichert-NOAA you had commented before about whether the print statements are necessary in the autoreconf sections. They are back in now after putting in @climbfuji's fixes. If you feel strongly about removing them, I'm fine doing that. |
I didn't realize I still had the print statements in there, sorry. Please remove them if you like. |
The print statements have been removed. |
I have a weird problem with my browser that doesn't allow me to approve PRs on github.com, therefore approving it this way. |
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.
@hppritcha and or @naughty please review.
This PR adds a new variant ("two_level_namespace") which has been used at JCSDA and partner organizations to solve a number of MacOS issues. The variant is off (False) by default and when enabled, it patches the configure script to enable the building of the openmpi package on MacOS using two level namespace.
This openmpi issue discusses the two level namespace configuration for MacOS: open-mpi/ompi#12427. We are dealing with the CommandLineTools 15.x (and newer) deprecation of "commons, use_dylibs" linker control by using the global linker option "-ld_classic". It looks like openmpi@5.0.4 may contain fixes for this issue and we can probably clean up (e.g., limit the application of the patching) this two_level_namespace variant once we move to 5.0.4 (we are currently on 5.0.3 at JCSDA).
Partially addresses: JCSDA/spack-stack#1140