Skip to content

stdenv/check-meta: add a warning for maintainerless derivations#170992

Merged
Mindavi merged 3 commits intoNixOS:masterfrom
ckiee:stdenv-maintainerless-warn
May 3, 2022
Merged

stdenv/check-meta: add a warning for maintainerless derivations#170992
Mindavi merged 3 commits intoNixOS:masterfrom
ckiee:stdenv-maintainerless-warn

Conversation

@ckiee
Copy link
Member

@ckiee ckiee commented Apr 30, 2022

Description of changes

This makes stdenv.mkDerivation emit warnings when a package does not
have any maintainers in order to encourage these core packages to be adopted.

Evaluating pkgs.hello currently results in 12 warnings, demonstrating
the need for this.

This behavior is gated behind a showDerivationWarnings nixpkgs config option
to reduce unnecessary output for end users.

Example output:

ckie@cookiemonster ~/git/nixpkgs -> nix-build --expr '(import ./. { config = {showDerivationWarnings = true;};}).hello'
trace: Package ed-1.18 in /home/ckie/git/nixpkgs/pkgs/applications/editors/ed/default.nix:23 has no maintainers, continuing anyway.
trace: Package patch-2.7.6 in /home/ckie/git/nixpkgs/pkgs/tools/text/gnupatch/default.nix:44 has no maintainers, continuing anyway.
[...]
/nix/store/g124820p9hlv4lj8qplzxw1c44dxaw1k-hello-2.12

cc @lheckemann, thanks for the idea.

TODO
  • Figure out why a warning for python3-minimal-3.9.12 is emitted even though it does have a maintainer Not block-worthy
  • Add a way to suppress warnings?

@github-actions github-actions bot added the 6.topic: stdenv Standard environment label Apr 30, 2022
@ckiee ckiee force-pushed the stdenv-maintainerless-warn branch from e4a9104 to 569ace0 Compare April 30, 2022 07:18
@vcunat
Copy link
Member

vcunat commented Apr 30, 2022

I don't know... this approach feels too annoying. Just look at the hello example. It's very trivial package, but there already you get warnings for many packages that intuitively don't much relate to the package. Also, it will bother on every evaluation, i.e. any time it's used; that's just not useful IMHO.

@vcunat
Copy link
Member

vcunat commented Apr 30, 2022

Maybe in some far-away future when the maintainers field is used very differently than nowadays. Another common issue is that one or more maintainers are listed but none has done anything around the package for years.

@mohe2015
Copy link
Contributor

Maybe instead we could add some easy way that allows you to enable this if you want. So you can easily check your own derivations. Dont know how to do this exactly though

@ckiee ckiee force-pushed the stdenv-maintainerless-warn branch from 569ace0 to 38377dd Compare April 30, 2022 08:09
@ckiee
Copy link
Member Author

ckiee commented Apr 30, 2022

Maybe instead we could add some easy way that allows you to enable this if you want. So you can easily check your own derivations. Dont know how to do this exactly though (~@mohe2015)

I've now gated this behind a new showDerivationWarnings config option:

ckie@cookiemonster ~/git/nixpkgs -> nix-build --expr '(import ./. { config = {showDerivationWarnings = true;};}).hello'
trace: Package python3-minimal-3.9.10 in /home/ckie/git/nixpkgs/pkgs/development/interpreters/python/cpython/default.nix:491 has no maintainers, continuing anyway.
[..snip..]
/nix/store/g124820p9hlv4lj8qplzxw1c44dxaw1k-hello-2.12

I don't know... this approach feels too annoying. Just look at the hello example. It's very trivial package, but there already you get warnings for many packages that intuitively don't much relate to the package. Also, it will bother on every evaluation, i.e. any time it's used; that's just not useful IMHO.

Well, if we want to get closer to addressing the considerable amount of orphans we need something like this.

Maybe in some far-away future when the maintainers field is used very differently than nowadays. Another common issue is that one or more maintainers are listed but none has done anything around the package for years.

Strawman? Addressing everything in one PR is not feasible.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 30, 2022
@roberth
Copy link
Member

roberth commented May 2, 2022

Please add to pkgs/top-level/config.nix.

See #166792 #171163

@ckiee
Copy link
Member Author

ckiee commented May 2, 2022

(@roberth)

Would you be open to a PR that documents all the other missing options I can think of, or is that already in the works?

@ckiee ckiee force-pushed the stdenv-maintainerless-warn branch 3 times, most recently from 6169ee7 to 3c60f1e Compare May 2, 2022 09:32
@roberth
Copy link
Member

roberth commented May 2, 2022

@ckiee I don't really have time for "big" projects, so that would be very much appreciated.

@Artturin
Copy link
Member

Artturin commented May 2, 2022

i'll add allowUnfree there

EDIT: #171248

@SuperSandro2000
Copy link
Member

Turning that on right now.

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Not sure about the valid changes mixed in here. But the warning for missing maintainers sounds like it could be useful for people interested in that. Personally I'm not looking forward to a wall of warnings, but I guess others may be.

Please at least split the valid change into a different commit, and reconsider making a separate PR for it.

@ckiee ckiee force-pushed the stdenv-maintainerless-warn branch from ea4951f to a48f638 Compare May 3, 2022 19:19
@ckiee
Copy link
Member Author

ckiee commented May 3, 2022

Not sure about the valid changes mixed in here.

valid is not exposed as a public API and breaking the one other relevant open PR seems unlikely and is not a blocker regardless. Hence this is a moot point.

Please at least split the valid change into a different commit, and reconsider making a separate PR for it.

I'm afraid you're a bit late for a separate PR (note how far this one has gone already) but I have split it into three commits with nicer, more detailed commit messages.

Also I just noticed there's merge conflicts again, on that.

ckiee added 3 commits May 3, 2022 22:28
This will allow for adding more validity types in the future, such as a
warning type. (which is in the next commit in this series)

This is NOT a breaking change because validity.valid is never exposed
outside of `stdenv.mkDerivation`.
This will be used in the next commit in this patch series.
This warning logs when a package has no maintainers. It will stay silent
if `meta.maintainers` is not set at all, only complaining when it is an
empty list. In the future a separate warning could be added to allow for
that stricter behavior. Or this warning could be changed.
@ckiee ckiee force-pushed the stdenv-maintainerless-warn branch from a48f638 to 4def222 Compare May 3, 2022 19:29
@Mindavi
Copy link
Contributor

Mindavi commented May 3, 2022

Good enough for me, thanks ✨ :

@ckiee
Copy link
Member Author

ckiee commented May 3, 2022

@Mindavi Merge after Ofborg's happy?

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

Labels

6.topic: stdenv Standard environment 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants