check-meta.nix, lib/types.nix: type check changes, more documentation

[piegamesde]

Description of changes

• Document that the checkMeta type check does not really work as intended
• Enable it by default nevertheless cause it's better than nothing, THIS IS A BREAKING CHANGE
• Small changes to lib/types.nix that are related

- Enable metadata checking by default, see #25304 (comment)
- Check metadata before any other package issues, see #191124 (comment)
- TODO fix soundness, see #6794 (comment)
- We really need to get that one fixed but I don't know too much about the internals, help appreciated here
- Also, the error message is currently as helpful as "has a value of invalid type set; expected list of submodule", I'd really like to get them to the same quality as what we have in the NixOS module system.

CC @ncfavier @copumpkin @nbp

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 22.11 Release Notes (or backporting 22.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[ncfavier]

Second point LGTM.

I think the first point deserves its own PR as it's a potentially breaking change with a big performance impact (I haven't read the linked issue in its entirety, but the fact that there was this much discussion tells me that it's not a trivial change to make, even 5 years later, and probably needs more discussion).

For the third and fourth points, see my suggestion at #6794 (comment).

[piegamesde]

As far as I can tell both the first and the third point are kind of breaking changes. As far as I can tell the default was left off in fear of breaking evaluation, but at some time we need to get on and break some eggs. It didn't automatically get better over five years, so we shouldn't expect it to happen in the future.

[piegamesde]

I pushed a type system experiment, please take a look. I think it is a step in the right direction and also the error messages are better. The only missing parts are a way to force the evaluation of all fields (currently hacked with builtins.trace builtins.toJSON) and a way to inject the package name into the error message.

CC @sternenseemann

[sternenseemann]

Overall, I don't like the module system type checking approach, I'd much rather just add new, stricter type variants to lib.types (e.g. lib.types.strictListOf or some better name) checks deeply and has a documented caveat about its performance.

[piegamesde]

Using the modules system is the least bad option I found: it takes care of default values, it can handle deep nesting, it gives us pretty decent error messages, it allows us to add example values and descriptions to meta attributes and I admit that I genuinely have no idea what half of the current values do.

I'm open to ideas on how to build type checks, but whatever we do they should work with composite types and I don't think this is currently possible without the module system or a lot of custom code. (The status quo simply iterates over all attributes, which totally ignores nested values)

[tazjin]

I'm open to ideas on how to build type checks, but whatever we do they should work with composite types and I don't think this is currently possible without the module system

Maybe you're talking about something else, but yants does not use the module system at all.

[SuperSandro2000]

@ofborg eval

[piegamesde]

@tazjin yants looks really impressive and I'd love to see it in nixpkgs. The restriction for the current change is to use existing type checking infrastructure, and under that I don't see many good alternatives to a module system type check.

[Profpatsch]

I’d advise against setting meta.check to true by default, partially because ofborg runs the check.

I’m afraid it will negatively influence eval times by ~a lot, do we have benchmarks of disabled/enabled/enabled new impl?

[piegamesde]

I know of the performance cost issue, and even though I don't have exact numbers yet I assume that it will be noticeable. However, a type system that is not checked is worth nothing. So, either we do keep the type checks and also run them in our CI, or we go back to the usual untyped madness and remove metaTypes entirely. Sacrificing soundness for performance reasons like we do at the moment is not an option.

[7c6f434c]

However, a type system that is not checked is worth nothing.

That's false. It's useful documentation.

Sacrificing soundness for performance reasons like we do at the moment is not an option.

If it were an actual policy, we would be quite limited in our compiler options for C++

Overloading OfBorg eval (which seems even harder to scale than builds) would be a much larger problem than what problems this change would solve.

[piegamesde]

I give up. I initially just stumbled upon this while implementing RFC 127 and tried to improve things a bit. I'd never planned to sink so much time into this, although the handful of closed previous attempts at fixing this should have warned me.

At the very least, I am going to document this madness so that the next person who runs into it knows what they are getting themselves into beforehand.

[piegamesde]

@sternenseemann do you have some examples for custom downstream meta attributes?

[tazjin]

The TVL CI system (which is used by, well, TVL, and quite a few companies using Nix) uses meta.ci as the key under which CI for a target is controlled. As sterni points out, it's also used in Hydra instances.

Other Nix-based CI systems might also do something like this.

[piegamesde]

Okay, I see the following possibilities how to fix this:

• Tell people to disable checkMeta when evaluating packages with custom attributes
• Move all those custom attributes off meta into passthru
• Add some unchecked metaPassthru or metaExtra attribute
• Let checkMeta allow custom attributes in general
• Add config.allowCustomMetaAttrs or something which defaults to true
• Make config.checkMeta default to false again
• (Edit, added later on): Add meta.dontCheckMeta to opt out in individual derivations

[sternenseemann]

Note also that the current error message tells users absolutely nothing about how to get rid of it.

[tazjin]

For what it's worth, I think any workaround that requires changes to each package definition is bad and creates useless toil.

[Profpatsch]

I’d advise against setting meta.check to true by default, partially because ofborg runs the check.

I’m afraid it will negatively influence eval times by ~a lot, do we have benchmarks of disabled/enabled/enabled new impl?

Quoting myself here, I suggest we turn off default meta checks again. As far as I can see, no benchmarks regarding eval time have been done, either.

[piegamesde]

I'm pretty sure OfBorg already runs the check because when the default was false somebody had to do the checks. The performed checks are minimal compared to my initial proposal (iterate over one attrset with a shallow type check vs a full module system instantiation); benchmarks are not on my priority list right now.

Regarding the breakage this caused, this can easily be worked around by manually specifying checkMeta = false; until all outstanding issues have been resolved.

[Profpatsch]

Regarding the breakage this caused, this can easily be worked around by manually specifying checkMeta = false; until all outstanding issues have been resolved.

Once again: No, that is not how upstream should act. Upstream shouldn’t break users and then tell them “just use a temporary solution until we get our shit together”.

As Linus states it: “We do not break Userspace. Period.”

[alyssais]

Nixpkgs has never agreed to guarantee stability of any API out-of-tree users might be using, and without any distinction between internal and public APIs, providing one would be infeasible.

I don't have an opinion on this change in particular, but Nixpkgs is not the kernel, and it's not even like the kernel, due to the lack of a userspace/kernelspace distinction.

[sternenseemann]

Yes, thanks to changes like this, you have to treat nixpkgs as everything is kernel space, as it is often unreasonably hard to figure out changes without actively tracking development of nixpkgs itself. Case in point:

• nixpkgs has now started rejecting derivations defined downstream that it accepted before (even derivations produced by tools provided by nixpkgs itself, see check-meta: Add isHydraChannel #193310!).
• The feature that rejects the derivations is barely documented now and was completely undocumented before, so users don't know it.
• The error message does not tell you what part of nixpkgs causes this error and how to disable the check. Downstream users will not be aware that the check is optional, unless they dig through the backtrace or bisect nixpkgs.
• Even worse: There is no clear reason for forcing this check upon downstream users – having arbitrary extra stuff in meta is unproblematic.

Overall, the question is not »Can we break userspace?«, because we can, but whether we should.

[Profpatsch]

This very much reminds me of the discussion under #177318, which broke downstream users for weeks, if not months, and caused wide-reaching bugs that the original author did not even anticipate.

[Profpatsch]

At least in this case it “just” causes evaluation failures, and not hard-to-debug runtime locale problems, but the annoyance exists nonetheless.

[piegamesde]

I'd like to point out that the reason why we are in this situation in the first place is that meta was an untyped mess without any checks for so long. Enabling security guards after the fact will never result in a smooth transition.

Going back and reverting this change is not an option. Instead, I'd like to have a constructive discussion about ways to bring this forward. I personally have no use cases that involve custom meta attributes, therefore I have no particular preferences for one above the other.

[Profpatsch]

Going back and reverting this change is not an option.

Yes it is, it is precisely an option. One with a default value that we can flip.

[sternenseemann]

I'd like to point out that the reason why we are in this situation in the first place is that meta was an untyped mess without any checks for so long. Enabling security guards after the fact will never result in a smooth transition.

This was only a problem for us, i.e. nixpkgs & nixos-search, no one else per se.

[Profpatsch]

Going back and reverting this change is not an option.

Yes it is, it is precisely an option. One with a default value that we can flip.

I’ve done it in #194058 which I strongly suggest we apply until further testing is in place.

[7c6f434c]

Enabling security guards after the fact will never result in a smooth transition.

Can you please explain how it is a security guard? What issue is prevented by forbidding extra attributes in meta?

[piegamesde]

I'm unsubscribing from the discussion now. I spent a considerable amount of time trying to improve things, only to be met by – mostly unconstructive, IMO – criticism, and then ended up spending a multiple of that time in unproductive discussions. I am saddened by the fact that people prefer to cling to the status quo instead of envisioning a solution that allows checkMeta to be enabled by default and custom meta attributes outside of nixpkgs.

This is not the first time this has happened to me in nixpkgs, and the next time I start some project in the attempt to improve things I will think about it twice beforehand.

[Profpatsch]

I can tell you exactly how to make this change without breaking downstream, but I don’t think I was ever asked. Rather, my objections were ignored as irrelevant and then people started complaining that their setups broke.

Some approaches:

• Don’t flip the flag outright, set it to "warn" and display a warning when the typecheck fails, with a link to some nice instructions how to fix this.
• Change the check to allow extra fields in meta; this could potentially clash with future field additions of course, but it would not break any downstream right now and could be refined at a later date.

[mweinelt]

The proposals made in #191171 (comment) were ignored all the same.

[tazjin]

It seems like

Let checkMeta allow custom attributes in general

Is the same as

Change the check to allow extra fields in meta

Which seems pretty sane to me, so maybe we should do that?

[mweinelt]

As mentioned by @alyssais in #194058 (comment) we would like to have stronger guarantees for nixpkgs.

Wouldn't it be sufficient to allow extra arguments in meta and enforce checks only on well-defined keys used throughout nixpkgs?

Upon reflection I don't think this is a good idea. The main use I've had of the meta checks in the past has actually been catching misspelled keys. So I think we really need a config option with an allow list of unrecognized meta checks.

Enumerating that allow list should not be too difficult? And adding to it should also be somewhat straightforward.

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixpkgs-architecture-team-meeting-12-agenda/22212/1

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixpkgs-architecture-team-meeting-12-agenda/22212/2

[nixos-discourse]

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/the-module-system-is-dead-how-to-do-input-validation/30297/1