Skip to content
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

Merged
merged 11 commits into from Feb 5, 2018
Merged

Conversation

msimberg
Copy link
Contributor

Fixes #2977.

Proposed Changes

  • Replace abstract topology class with the hwloc topology class (I have only moved over the functions, not done any other structural changes)
  • Update documentation with some basic instructions on getting hwloc
  • Remove cmake options for disabling hwloc
  • Remove some unused functions definitions/declarations

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)?

hwloc>`) while executing cmake for __hpx__ as explained in detail in the
sections __unix_installation__ and __windows_installation__.

[endsect]
Copy link
Member

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.

Copy link
Contributor Author

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)?

Copy link
Member

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.

Copy link
Contributor Author

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]]
Copy link
Member

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)?

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

Copy link
Member

@hkaiser hkaiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@hkaiser
Copy link
Member

hkaiser commented Jan 31, 2018

This also fixes #2996 and #3009.

Point to generic docs page, not to a specific version
@msimberg msimberg mentioned this pull request Jan 31, 2018
12 tasks
@hkaiser
Copy link
Member

hkaiser commented Feb 4, 2018

@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

#include <hpx/config/warnings_prefix.hpp>
// your type goes here
#include <hpx/config/warnings_suffix.hpp>

(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.

@msimberg
Copy link
Contributor Author

msimberg commented Feb 5, 2018

@hkaiser I've added the includes and appveyor seems happy now. Thanks for pointing that out!

@hkaiser
Copy link
Member

hkaiser commented Feb 5, 2018

@msimberg Thanks!

@msimberg msimberg merged commit 1da6c95 into STEllAR-GROUP:master Feb 5, 2018
@msimberg msimberg deleted the hwloc-compulsory branch February 6, 2018 08:26
This was referenced Feb 6, 2018
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.

None yet

3 participants