-
-
Notifications
You must be signed in to change notification settings - Fork 19.4k
meta.problems: Custom package problems [RFC127] #478539
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
Changes from all commits
7ae278d
60ef776
27fcf5c
2032a6b
8fe8f0e
27dd434
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,6 +11,8 @@ By default, Nix will prevent installation if any of the following criteria are t | |
|
|
||
| - The package has known security vulnerabilities but has not or can not be updated for some reason, and a list of issues has been entered into the package's `meta.knownVulnerabilities`. | ||
|
|
||
| - There are problems for packages which must be acknowledged, e.g. deprecation notices. | ||
|
|
||
| Each of these criteria can be altered in the Nixpkgs configuration. | ||
|
|
||
| :::{.note} | ||
|
|
@@ -166,6 +168,57 @@ There are several ways to tweak how Nix handles a package which has been marked | |
|
|
||
| Note that `permittedInsecurePackages` is only checked if `allowInsecurePredicate` is not specified. | ||
|
|
||
| ## Packages with problems {#sec-problems} | ||
|
|
||
| A package may have several problems associated with it. | ||
| These can be either manually declared in `meta.problems`, or automatically generated from its other `meta` attributes. | ||
| Each problem has a name, a "kind", a message, and optionally a list of URLs. | ||
| Not all kinds can be manually specified in `meta.problems`, and some kinds can exist only up to once per package. | ||
| Currently, the following problem kinds are known (with more reserved to be added in the future): | ||
|
infinisil marked this conversation as resolved.
|
||
|
|
||
| - "removal": The package is planned to be removed some time in the future. Unique. | ||
|
infinisil marked this conversation as resolved.
|
||
| - "deprecated": The package relies on software which has reached its end of life. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two questions:
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As already replied in #478539 (comment), I'd rather not try to specify how this should be used exactly. For now I just want to make sure the mechanism works as the RFC specifies. |
||
| - "maintainerless": Automatically generated for packages with `meta.maintainers == []`. Unique, not manually specifiable. | ||
|
|
||
| Each problem has a handler that deals with it, which can be one of "error", "warn" or "ignore". | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: I think I'd prefer if this was called "severity". Declaring what it is rather than what should be done with it feels more right to me.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Handler" also sort of implies that what's declared is an action or a command; perhaps even a custom one. "Severity" OTOH is just a property of the problem itself.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like the idea. It's a derivation from the RFC, but I don't think we need to follow it to the dot.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Although, this also implies that we should change |
||
| "error" will disallow evaluating a package, while "warn" will simply print a message to the log. | ||
|
|
||
| The handler for problems can be specified using `config.problems.handlers.${packageName}.${problemName} = "${handler}";`. | ||
|
|
||
| There is also the possibility to specify some generic matchers, which can set a handler for more than a specific problem of a specific package. | ||
| This works through the `config.problems.matchers` option: | ||
|
|
||
| ```nix | ||
| { | ||
| problems.matchers = [ | ||
| # Fail to build any packages which are about to be removed anyway | ||
| { | ||
| kind = "removal"; | ||
| handler = "error"; | ||
| } | ||
|
|
||
| # Get warnings when using packages with no declared maintainers | ||
| { | ||
| kind = "maintainerless"; | ||
| handler = "warn"; | ||
| } | ||
|
|
||
| # You deeply care about this package and want to absolutely know when it has any problems | ||
| { | ||
| package = "hello"; | ||
| handler = "error"; | ||
| } | ||
| ]; | ||
| } | ||
| ``` | ||
|
|
||
| Matchers can match one or more of package name, problem name or problem kind. | ||
| If multiple conditions are present, all must be met to match. | ||
| If multiple matchers match a problem, then the highest severity handler will be chosen. | ||
| The current default value contains `{ kind = "removal"; handler = "warn"; }`, meaning that people will be notified about package removals in advance. | ||
|
|
||
| Package names for both `problems.handlers` and `problems.matchers` are taken from `lib.getName`, which looks at the `pname` first and falls back to extracting the "pname" part from the `name` attribute. | ||
|
|
||
| ## Modify packages via `packageOverrides` {#sec-modify-via-packageOverrides} | ||
|
|
||
| You can define a function called `packageOverrides` in your local `~/.config/nixpkgs/config.nix` to override Nix packages. It must be a function that takes pkgs as an argument and returns a modified set of packages. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally posted by @ConnorBaker in #478539 (comment)
I'd love some help!
Most of the CUDA packages come from binary archives provided by NVIDIA (we call these "redists" because we found them at a URL with "redist" in the path, the presence of a "redistrib_.json" manifest, and the LICENSE for some of them allowing redistribution). We have a generic
buildRedistfunction we use to make packages from those binary archives, defined here: https://github.com/NixOS/nixpkgs/blob/9774bb59bd3bb978cf86b38afc70f9c6d5505983/pkgs/development/cuda-modules/buildRedist/default.nix.Since availability of CUDA packages depends on platform, CUDA version, CUDA capabilities (think the type of GPU), and versions of dependencies, there's a lot of ways things can break (either when trying to patchelf the binary or at runtime). Making matters more complicated, NVIDIA does a fair amount of optimistic binary loading through dlopen calls at runtime, depending on the code path and the environment (e.g., CUDA version, CUDA capability, platform, etc.).
We try to fail early (during evaluation) when possible to avoid very expensive (time, space, and compute) builds that would fail or produce binaries things known to fail. To do that, we have two different attributes set through
passthrubybuildRedist:brokenAssertionsandplatformAssertions.brokenAssertionsis defined here:nixpkgs/pkgs/development/cuda-modules/buildRedist/default.nix
Lines 388 to 425 in 9774bb5
and
platformAssertionsis defined here:nixpkgs/pkgs/development/cuda-modules/buildRedist/default.nix
Lines 427 to 446 in 9774bb5
Then, in
meta,buildRedistuses the final copies of those values to setmeta.brokenandmeta.badPlatforms:nixpkgs/pkgs/development/cuda-modules/buildRedist/default.nix
Lines 455 to 456 in 9774bb5
The helper functions produce the expected values, additionally printing out failed assertions through
builtins.traceVerbose: https://github.com/NixOS/nixpkgs/blob/3f96296da66f5ecf3d8106c61281b823949a56c0/pkgs/development/cuda-modules/_cuda/lib/meta.nix.As an example of how this is used, look at cuDNN, which uses
platformAssertionsto ensure it is only available for supported CUDA capabilities:nixpkgs/pkgs/development/cuda-modules/packages/cudnn.nix
Lines 52 to 104 in 9774bb5
I'd love to see how (or if) the current implementation of the problems RFC would address such a use case.
As an added bonus, I'd love to see the definition for
cudaPackages.backendStdenvsimplified: https://github.com/NixOS/nixpkgs/blob/9774bb59bd3bb978cf86b38afc70f9c6d5505983/pkgs/development/cuda-modules/backendStdenv/default.nix.Beyond ensuring a compatible version of GCC (patched to use glibc/libstdc++ from the default stdenv) is available for NVCC, it also performs a fair amount of logic in determining the default set of CUDA capabilities for some version of CUDA as well as whether explicitly requested CUDA capabilities are valid for the given CUDA version. It doesn't make sense to attach these through
buildRedistsince it affects all CUDA packages, and causing evaluation ofbackendStdenvto fail also allows us to fail evaluation in the presence of incorrect configurations (since a number of the utilities incudaPackages.flagsare derived fromcudaPackages.backendStdenv).Originally posted by @infinisil in #478539 (comment)
@ConnorBaker Thanks for the detailed explanation! I've created another draft PR based on the one here to implement a
brokenerror kind: tweag#111With that, it should be possible for you to do this for assertions:
We could also consider adding a generic
enable/condition/assertionfield to problems to make this more ergonomic.If an assertion is wrong, it causes evaluation to fail with the message displayed, along with info on how specific assertions can be switched to just warn or be entirely ignored.
I believe this would be appropriate for both the current
brokenAssertionsandplatformAssertions, while not setting anymeta.brokenandmeta.badPlatformsvalues. I plan to make a PR switching all of them for you to test, but early feedback is also appreciated :)Originally posted by @infinisil in #478539 (comment)
@ConnorBaker Can you check out tweag#112?
Originally posted by @ConnorBaker in #478539 (comment)
I had a chance to look at tweag#111 and tweag#112 and I like what you've done!
I personally would like to see that, or something which aligns the attributes with the NixOS module system assertions/warnings.
In testing out tweag#112, I noticed it seems there's a double newline before "Package problems":
Is that intentional?
I'm glad to see these impact
meta.available(which I use in some places to handle different configurations).What do you imagine the use-case for the
unsupportedkind being and how would you say it differs frombroken?Originally posted by @infinisil in #478539 (comment)
Noted, but probably better in a follow-up PR to keep the scope of this one smaller.
Nope. Really the error format needs some tweaking in general imo, it's too big and contains duplicate info. How about this instead:
That's actually a very good point. These have very different notions:
We'd want to use both of these for CUDA
Originally posted by @ConnorBaker in #478539 (comment)
Ah, that makes sense. I worry that unsupported may not exactly align with how we'd want to use it for CUDA -- that is, to explain that the user has selected a configuration which will not or cannot work and that there is no upstream fix that would enable it. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ConnorBaker Right, I see! Then I think it would be best to introduce a separate
assertionproblem kind for generic internal assertions that have been violated, not necessarily relating to broken or unsupported.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the reason I've done broken or unsupported system previously is to indicate or communicate that such a configuration just isn't possible and that even more than "this will fail to build" the user might get evaluation errors (e.g., trying to do a lookup in an attribute set for a CUDA binary for an unsupported platform, like Darwin, will throw a missing attribute exception) -- kind of like a "stop digging! nothing you want is here!".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For those cases I actually think a simple
foo.attrThatMightNotExist or (throw "This does not work!")would be best, becausemeta.problemswill inherently allow users to ignore the problem and try anyways, which doesn't make a lot of sense if we know exactly where during evaluation it will fail.So I think of
meta.problemsas a way for package maintainers to say thatUh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A better message could be a warning that says:
So with a package being like nim1 and problem being it doesn't exist, recommend nim2 & problems may include security vulnerabilities, package building issue.
Would look like:
Because I find the message that you mentioned @infinisil having a lot of idioms personally.