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

procps: enable systemd support when available #43817

Merged
merged 1 commit into from Aug 30, 2018
Merged

procps: enable systemd support when available #43817

merged 1 commit into from Aug 30, 2018

Conversation

binarin
Copy link
Contributor

@binarin binarin commented Jul 19, 2018 •
edited

Motivation for this change

ps can show various systemd-related information, like a unit to which a process belongs. But when it's not compiled in, it shows only ? in such fields.

Can be tested with:

ps -o unit= ax
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

matthewbauer
matthewbauer reviewed Jul 19, 2018
View reviewed changes
pkgs/os-specific/linux/procps-ng/default.nix Outdated
@@ -1,4 +1,6 @@
{ lib, stdenv, fetchurl, ncurses, pkgconfig }:
{ lib, stdenv, fetchurl, ncurses, pkgconfig
, systemd ? null
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 not the correct way to do this. For instance on macOS systems systemd will not be null - it just won't build because it is linux-only. The closest to a correct solution is something like this:

{ lib, stdenv, fetchurl, ncurses, pkgconfig
, withSystemd ? stdenv.isLinux, systemd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@binarin
Copy link
Contributor Author

binarin commented Jul 19, 2018

Something strange happens with this change, it can be seen in GrahamcOfBorg error message above. And it actually breaks nixos VM tests, with the same message.

I've tried to look into this a bit, and it looks like things like fetchzip and fetchpatch are getting fetchurlBoot instead of full-fledged fetchurl (where support for all the fancy options really is).

@binarin
Copy link
Contributor Author

binarin commented Jul 20, 2018

I don't know how to investigate that fetchurl/bootstrap issue. So for now I've made this option available only by explicit request, so that #34265 can move forward.

@binarin binarin mentioned this pull request Jul 20, 2018
Merged
8 tasks
matthewbauer
matthewbauer reviewed Jul 24, 2018
View reviewed changes
pkgs/os-specific/linux/procps-ng/default.nix Outdated
@@ -11,14 +22,16 @@ stdenv.mkDerivation rec {
};

buildInputs = [ ncurses ];
nativeBuildInputs = [ pkgconfig ];
nativeBuildInputs = [ pkgconfig ]
++ lib.optional withSystemd systemd;
Copy link
Member

Choose a reason for hiding this comment

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

This should be a build input.

@matthewbauer
Copy link
Member

I pushed a few changes. I could not reproduce the ofborg failure. It could have been something that was closed.

@binarin
Copy link
Contributor Author

binarin commented Jul 27, 2018

I've dug deeper into this issue, as VM tests were still failing. Short story is that during tests procps/systemd is evaluated with stdenv bootstrap-stage4-stdenv-linux that is not functional enough to build systemd.

As a quick hack I'm going to make a systemd dependency conditional not only on isLinux, but also on the absence of bootstrap in stdenv.name.
Or maybe somebody with more understanding of bootstrap stage layering, fix-points, etc. will chime in with a proper fix.

@binarin
procps: enable systemd support when available
86b0974 
`ps` can show various systemd-related information, like a unit to
which a process belongs. But when it's not compiled it, it shows only
'?' in such fields.

Can be tested with:

    ps -o unit= ax
@vcunat vcunat changed the base branch from master to staging August 30, 2018 16:05
@vcunat vcunat merged commit 86b0974 into NixOS:staging Aug 30, 2018
vcunat added a commit that referenced this pull request Aug 30, 2018
@vcunat
Merge #43817: procps(-ng): enable systemd support
2a39174 
The whole closure only grows by 5 MiB roughly,
and it's all very common libraries (very low practical impact).
@vcunat
Copy link
Member

vcunat commented Aug 30, 2018

Merged along with 5ae438a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer left review comments

@vcunat vcunat vcunat approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@binarin @matthewbauer @vcunat @GrahamcOfBorg