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.

Sign up for GitHub

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

Jump to bottom

openmpi: enable c++ bindings #94829

Merged
merged 1 commit into from Aug 7, 2020
Merged

openmpi: enable c++ bindings #94829

merged 1 commit into from Aug 7, 2020

Conversation

matthuszagh
Copy link
Contributor

@matthuszagh matthuszagh commented Aug 7, 2020 •
edited

Motivation for this change

Part of this pr. @jonringer

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@matthuszagh matthuszagh marked this pull request as draft August 7, 2020 00:16
@matthuszagh matthuszagh marked this pull request as ready for review August 7, 2020 00:21
@ofborg ofborg bot requested a review from 7c6f434c August 7, 2020 00:25
markuskowa
markuskowa reviewed Aug 7, 2020
View reviewed changes
pkgs/development/libraries/openmpi/default.nix Outdated
@@ -14,6 +14,8 @@
# Enable libfabric support (necessary for Omnipath networks) on x86_64 linux
, fabricSupport ? stdenv.isLinux && stdenv.isx86_64

# enable C++ bindings
, enableCpp ? false
Copy link
Member

@markuskowa markuskowa Aug 7, 2020 •
edited

Choose a reason for hiding this comment

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

We may not need to make it conditional but it can always be enabled. I will run nixpkgs-review on it to see if that affects other packages.

Copy link
Member

Choose a reason for hiding this comment

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

It is OK to enable it per default. Let's remove the conditional here and always enable it on Linux.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I've made the change.

@markuskowa markuskowa changed the title Openmpi openmpi: enable c++ bindings Aug 7, 2020
@markuskowa markuskowa self-assigned this Aug 7, 2020
@markuskowa markuskowa merged commit b9bf9c9 into NixOS:master Aug 7, 2020
@matthuszagh matthuszagh deleted the openmpi branch August 7, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@7c6f434c 7c6f434c Awaiting requested review from 7c6f434c

@markuskowa markuskowa Awaiting requested review from markuskowa

Assignees

@markuskowa markuskowa

Labels
10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@matthuszagh @markuskowa