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

rdma-core: fix rxe_cfg and platforms flags #34407

Merged
merged 3 commits into from Jan 31, 2018

Conversation

markuskowa
Copy link
Member

@markuskowa markuskowa commented Jan 30, 2018

Motivation for this change
  • Fix a path in rxe_cfg for persistent config data (script crashes when location in /nix/store)
  • proper path for ifconfig
  • Set platform flag to linux
Things done

rxe_cfg can now be used to add RDMA transport to network interface

  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

substituteInPlace $out/bin/rxe_cfg --replace ethtool "${ethtool}/bin/ethtool"
postPatch = ''
substituteInPlace providers/rxe/rxe_cfg.in \
--replace '@CMAKE_INSTALL_FULL_SHAREDSTATEDIR@' '/run' \
Copy link
Member

Choose a reason for hiding this comment

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

how about:

cmakeFlags = ["-DCMAKE_INSTALL_RUNDIR=/run"];

instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

That does not seem to work. @CMAKE_INSTALL_FULL_SHAREDSTATEDIR@ still gets substituted by the store path (e.g. /nix/store/vaxhll3n336v6rlh8ql2jqalq1hpykxw-rdma-core-16.1/com/rxe) when I set CMAKE_INSTALL_RUNDIR=/run. The cmake doc says that this should be substituted as PREFIX/com
(https://cmake.org/cmake/help/v3.8/module/GNUInstallDirs.html). Is there another way to override SHAREDSTATEDIR through cmake? I don't see it in the docs.

Copy link
Member

Choose a reason for hiding this comment

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

This is strange, when looking at this code: https://github.com/linux-rdma/rdma-core/blob/master/CMakeLists.txt#L93

overriding CMAKE_INSTALL_RUNDIR with an absolute path as part of cmake flags should just work.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK I see. I guess the problem is a different one here. The script uses SHAREDSTATEDIR, which ends up being PREFIX/com (this seems to be an odd choice to me). If it would use CMAKE_INSTALL_RUNDIR, then I guess your suggestions should work.

The reason I need to patch that is the following: rxe_cfg lets one choice if the config should be made persistent or not. Even when I select -n (no persistent) it still performs touch on the config file (which fails on a location in the nix store). From a nixos perspective the persistent choice should be made through a module. That's why I chose to set this path to /run.

Copy link
Member

@Mic92 Mic92 Jan 30, 2018

Choose a reason for hiding this comment

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

Sorry I mixed up variables -DCMAKE_INSTALL_SHAREDSTATEDIR=/var/lib should do the trick. So you would recommend /run instead?

Copy link
Member

@Mic92 Mic92 Jan 30, 2018

Choose a reason for hiding this comment

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

Unless it would breaks the module, I would keep it as the developer intended it. In future sharedstatedir might be use somewhere else also.

Copy link
Member

Choose a reason for hiding this comment

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

Also people might want to use it on non-nixos system - does this makes sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it does make sense. /var/lib is a good choice. Just checked your commit, it works. Thanks!

@Mic92 Mic92 merged commit 5877b81 into NixOS:master Jan 31, 2018
@markuskowa markuskowa deleted the rdma-core-fix-pr branch February 1, 2018 00:44
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