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

treewide: systemdSupport: use lib.meta.availableOn #192197

Merged
merged 3 commits into from Jan 22, 2023
Merged

treewide: systemdSupport: use lib.meta.availableOn #192197

merged 3 commits into from Jan 22, 2023

Conversation

ghost
Copy link

@ghost ghost commented Sep 21, 2022

Description of changes

Many packages have some kind of flag indicating whether or not to build with systemd support. Most of these default to stdenv.isLinux, but systemd does not build on (and is marked broken for) isStatic. Only a few packages have the needed && !isStatic in the default value for their parameter.

This PR moves the logic for the default value of these flags into systemd.meta.{platforms,badPlatforms} and evaluates those conditions using lib.meta.availableOn.

This provides three benefits:

  1. The default values are set correctly (i.e. including && isStatic)
  2. The default values are set consistently
  3. The way is paved for any future non-Linux systemd platforms (FreeBSD is reported to have experimental systemd support)
Things done

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 labels Sep 21, 2022
@ghost ghost marked this pull request as ready for review September 21, 2022 07:44
@Ericson2314
Copy link
Member

I don't think this is a good idea. Systemd is an init system, that's way removed from an intrinsic property of the platform.

What we should instead have is something that works for any package, like

systemdSupport ? systemd.meta.canBuild

indeed, we used to have something like that. Not sure why it was removed.

@ghost ghost marked this pull request as draft September 23, 2022 03:58
@ghost

This comment was marked as resolved.

@ghost ghost changed the title lib/systems: add {host,build,target}Platform.isSystemd treewide: systemdSupport: use lib.meta.availableOn Sep 25, 2022
@ghost
Copy link
Author

ghost commented Sep 25, 2022

systemdSupport ? systemd.meta.canBuild

indeed, we used to have something like that. Not sure why it was removed.

I assume you mean something other than meta.platforms and meta.broken.

Ah, I see, you meant meta.availableOn: #114374

Please see latest push. In particular, it was necessary for 2daa429cc3205033a0ea7164aaff2a5117551fe7 to extend lib.meta.platformMatch to accept predicate functions, since isStatic cannot be defined structurally if pkgsStatic is involved.

@ghost
Copy link
Author

ghost commented Sep 25, 2022

Squashed.

@ghost ghost marked this pull request as ready for review September 25, 2022 06:30
@ghost ghost requested review from a team, edolstra and infinisil as code owners September 25, 2022 06:30
@ofborg ofborg bot requested review from Ekleog, fadenb and kevincox and removed request for a team September 25, 2022 06:40
Many packages have some kind of flag indicating whether or not to build with
systemd support.  Most of these default to `stdenv.isLinux`, but systemd does
not build on (and is marked `broken` for) `isStatic`.  Only a few packages have
the needed `&& !isStatic` in the default value for their parameter.

This commit moves the logic for the default value of these flags into
`systemd.meta.{platforms,badPlatforms}` and evaluates those conditions using
`lib.meta.availableOn`.

This provides three benefits:

1. The default values are set correctly (i.e. including `&& isStatic`)

2. The default values are set consistently

3. The way is paved for any future non-Linux systemd platforms (FreeBSD is
   reported to have experimental systemd support)
@ghost
Copy link
Author

ghost commented Jan 22, 2023

Resolved merge conflict.

@flokli flokli merged commit d99d2ce into NixOS:master Jan 22, 2023
@flokli
Copy link
Contributor

flokli commented Jan 22, 2023

Thanks for your patience, and sorry this took so long!

@ghost ghost deleted the pr/system/isSystemd branch January 22, 2023 19:46
chvp added a commit to chvp/nixpkgs that referenced this pull request Jan 24, 2023
…temd

NixOS#192197 broke these packages by adding
systemd as a dependency. This meant that the included package was no longer the
python3 systemd package, but the general systemd derivation. This broke the
packages at runtime. This PR fixes that.
@ncfavier
Copy link
Member

ncfavier commented Jan 24, 2023

Allowing functions in meta.{platforms,badPlatforms} isn't great for discoverability: nix-env checks meta attributes and will output "platforms": null if platforms contains a function, which in turn means that the package will have no platforms listed on search.nixos.org.

Can isStatic be made into a pattern instead? We still have to figure out how to display those but at least this wouldn't kill the whole attribute.

In any case I think the documentation for lib.meta.platformMatch should mention that using functions there is discouraged.

@ghost
Copy link
Author

ghost commented Jan 25, 2023

Allowing functions in meta.{platforms,badPlatforms} isn't great for discoverability

Yeah, and Nix's fragile function equality is not something we should expand our reliance upon. @sternenseemann has documented the full extent of how crazy it is, which goes way beyond just f != (x: f x). I opened this PR before that situation was fully understood.

Can isStatic be made into a pattern instead?

Not trivially. But I do think it is possible and was thinking of attempting it. @ncfavier would you be willing to review the result? If it turns out to be possible, I will include a revert of b7d0974 in that PR. Handling isStatic is the only justification for functions in meta.platforms.

@ncfavier
Copy link
Member

@sternenseemann has documented the full extent of how crazy it is

Jesus. 🤯

would you be willing to review the result

Willing, sure. Capable, we'll see.

Handling isStatic is the only justification for functions in meta.platforms.

How about the stuff in lib.systems.inspect.predicates? Do they all map to patterns cleanly?

@ghost
Copy link
Author

ghost commented Jan 25, 2023

How about the stuff in lib.systems.inspect.predicates? Do they all map to patterns cleanly?

Yes; in fact the predicate functions are generated from the patterns.

Except for isStatic. It's the oddball.

Mainly because it's the only part of a hostPlatform that isn't determined by the gnu-config tuple (e.g. x86_64-unknown-linux-gnu).

@SuperSandro2000
Copy link
Member

The python breakages where already fixed in #212391 and the other two things I fixed in #212613

@SuperSandro2000
Copy link
Member

I don't like seeing this change and generally more often depending on lib.meta.availableOn for compile options because if we adjust platforms for a package we can silently disable features in other packages which where before expected to be enabled. This can't be easily be caught in review and I am expecting this to be the cause of unexpected breakages in the future.

ckiee pushed a commit to ckiee/nixpkgs that referenced this pull request Jan 26, 2023
…temd

NixOS#192197 broke these packages by adding
systemd as a dependency. This meant that the included package was no longer the
python3 systemd package, but the general systemd derivation. This broke the
packages at runtime. This PR fixes that.
@trofi
Copy link
Contributor

trofi commented Jan 27, 2023

nix-env does not support predicates and assumes plain strings. As a result nixpkgs-review complains at every review as has invalid meta attribute 'badPlatforms':

$ nix-env --option system x86_64-linux -f /home/slyfox/.cache/nixpkgs-review/pr-212919/nixpkgs -qaP --xml --out-path --show-trace --no-allow-import-from-derivation --meta
derivation 'systemd-252.4' has invalid meta attribute 'badPlatforms'
derivation 'systemd-minimal-252.4' has invalid meta attribute 'badPlatforms'
derivation 'systemd-stage-1-252.4' has invalid meta attribute 'badPlatforms'
derivation 'systemd-stage-1-network-252.4' has invalid meta attribute 'badPlatforms'

I'm surprised to see meta attributes not being plain values. It's not metadata if you need to evaluate it.

@ghost
Copy link
Author

ghost commented Jan 27, 2023

#212925

nix-env does not support predicates and assumes plain strings.

Only nix-env --meta is affected, not the rest of nix-env.

It's not metadata if you need to evaluate it.

In a lazy language like Nix everything needs to be evaluated:

$ nix eval --expr 'builtins.trace { x=(x:x) 3; } ""'
trace: { x = <CODE>; }

I think you meant to say something like "it's not metadata if you can't run builtins.toString on it". I'm not sure I agree with that either. Unfortunately nix-env --meta does, and it's part of the stable command set (nix-build, nix-instantiate, nix-env) so changing its behavior is the very last resort.

The long-term fix is to fix isStatic. I think I have a short-term fix in the meantime. See #212925

@ghost
Copy link
Author

ghost commented Jan 27, 2023

It's a bit rough, but this could be the long-term fix:

@alyssais
Copy link
Member

I don't like seeing this change and generally more often depending on lib.meta.availableOn for compile options because if we adjust platforms for a package we can silently disable features in other packages which where before expected to be enabled. This can't be easily be caught in review and I am expecting this to be the cause of unexpected breakages in the future.

I'm worried about package options that look at meta.broken, but I don't expect this to be a problem for meta.platforms, because as long as we're maintaining the conventional meanings of platforms being "where should this package work?", and broken being "where is this package not working because of bugs?", the only situation where we remove a platform from meta.platforms should be when we discover that the package doesn't work on that platform and was never intended to. In that case, disabling downstream optional features is exactly what we'd want to happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: emacs Text editor 6.topic: erlang 6.topic: printing 6.topic: stdenv Standard environment 6.topic: systemd 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 1-10 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants