Skip to content

Conversation

@jsquyres
Copy link
Member

Signed-off-by: Jeff Squyres jsquyres@cisco.com

Turns out that this code already exists in the v3.0.x branch.

FYI: @bgoglin @rhc54

Signed-off-by: Jeff Squyres <jsquyres@cisco.com>
@jsquyres jsquyres requested a review from rhc54 March 21, 2018 03:43
@jsquyres jsquyres changed the title hwloc/external: disallow hwloc >=v2.0.0 v3.1.x: hwloc/external: disallow hwloc >=v2.0.0 Mar 21, 2018
@bgoglin
Copy link
Contributor

bgoglin commented Mar 21, 2018

It's identical to my #4934 (except for one blank line) so I am going to assume it's OK :)

@ggouaillardet
Copy link
Contributor

note there is still a bunch of #if HWLOC_API_VERSION < 0x20000 is opal/mca/hwloc/base and you might want to remove this dead code as well.

@rhc54
Copy link
Contributor

rhc54 commented Mar 21, 2018

@ggouaillardet We don't want to remove the hwloc v2.0 integration code at this time. This patch is required because your "glue" was based on a pre-final version of hwloc v2.0, and there are changes which break it. So the plan is to first protect this release so someone using an external hwloc doesn't hit the problem, and then fix the integration to match the final hwloc v2 API.

@ggouaillardet
Copy link
Contributor

@rhc54 got it !

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.

Quibble: I'm not sure a user is going to understand that hwloc version number in the error message. Might be better to say "v2.0" as that is something they can understand.

@jsquyres
Copy link
Member Author

@bgoglin Somehow I missed your #4934. Doh! Let's close this one and take yours, since you were first.

@rhc54 I think you mis-read the patch. It emits checking if external hwloc version is lower than 2.0 to stdout, and then emits OMPI does not currently support hwloc v2 API / Cannot continue if hwloc is too high. The #error is inside the internal configure test.

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.

4 participants