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
Make hwloc compulsory #3133
Make hwloc compulsory #3133
Conversation
hwloc>`) while executing cmake for __hpx__ as explained in detail in the | ||
sections __unix_installation__ and __windows_installation__. | ||
|
||
[endsect] |
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.
Could you please add a note that the instructions don't refer to Windows based installations. I'm not sure about MacOS and/or Android based systems (@sithhell?). For Windows the user can simply download the 64Bit binaries from the download page.
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've added a note. Could you check that it's accurate (I wrote a one-sentence explanation for how to install hwloc on Windows)?
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 suggest to link to the hwloc documentation for further information:
https://www.open-mpi.org/projects/hwloc/doc/v1.11.9/
(There's no direct link to the installation section)
From a cursory look, it seems that getting hwloc to run on android is possible (but there's no real documentation), for macosx, I guess that autotools just works? Or homebrew or something similar.
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.
@sithhell I've added the hwloc link as a note at the end of the installation section.
docs/hpx.qbk
Outdated
@@ -90,7 +90,8 @@ | |||
[def __boost_atomic__ [@http://www.boost.org/doc/libs/release/doc/html/atomic.html Boost.Atomic]] | |||
[def __boost_serialization__ [@http://www.boost.org/doc/libs/release/libs/serialization/doc/index.html Boost.Serialization]] | |||
|
|||
[def __hwloc_downloads__ [@http://www.open-mpi.org/projects/hwloc/]] | |||
[def __hwloc_downloads__ [@http://www.open-mpi.org/software/hwloc/v1.11 Hwloc Downloads]] | |||
[def __hwloc_doc__ [@http://www.open-mpi.org/projects/hwloc/doc/v1.11.9 Hwloc Documentation]] |
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.
Should we link to https://www.open-mpi.org/projects/hwloc/doc/ instead (no version)?
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.
Good point, less updating for us. What about the download link? There doesn't seem to a page with all the downloads like for the docs. I think linking to the 1.11 download page is nicer in this case.
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.
Indeed.
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, thanks!
Point to generic docs page, not to a specific version
@msimberg This patch generates a ton of MSVC specific warnings complaining about dllexport conflicts (see https://ci.appveyor.com/project/hkaiser/hpx/build/master-1819/job/vu2x573ux94f50y3). Those usually can be safely ignored. Could you please wrap the definition of the new topology type using
(see for instance here how it's done: https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/runtime/threads/threadmanager.hpp#L41 and https://github.com/STEllAR-GROUP/hpx/blob/master/hpx/runtime/threads/threadmanager.hpp#L428, those have to be as close to the type itself as possible). These includes will disable the warnings as needed. |
@hkaiser I've added the includes and appveyor seems happy now. Thanks for pointing that out! |
@msimberg Thanks! |
Fixes #2977.
Proposed Changes
Any background context you want to provide?
hwloc was already required in practice (for performance reasons, and because the build was broken with hwloc disabled), so this just makes it officially required.
@biddisco could you have a look to see if I might've missed some details, please? Do we need to update the recommended/minimum version numbers (current recommended is 1.10, minimum is 1.5, latest is 1.11)?