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

[RFC 0127] Nixpkgs "problem" infrastructure #127

Merged
merged 25 commits into from
Jul 12, 2023
Merged
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
60cb23d
[RFC 0127] Nixpkgs issues and warnings (#127)
piegamesde Jun 11, 2022
ee72999
Add URLs as structured information
piegamesde Jun 15, 2022
c109c31
Update rfcs/0127-issues-warnings.md
piegamesde Sep 12, 2022
ff2e2d4
Update rfcs/0127-issues-warnings.md
piegamesde Sep 12, 2022
f83067a
Update rfcs/0127-issues-warnings.md
piegamesde Sep 13, 2022
2daf978
Update rfcs/0127-issues-warnings.md
piegamesde Sep 13, 2022
2b63424
Update rfcs/0127-issues-warnings.md
piegamesde Sep 13, 2022
e5c0e13
Update rfcs/0127-issues-warnings.md
piegamesde Sep 13, 2022
3212d31
RFC 127 update shepherds
piegamesde Sep 13, 2022
ecdbe2c
RFC 127 rework
piegamesde Sep 13, 2022
c907adb
Point out that the previous warnings system was not documented
piegamesde Sep 13, 2022
d232a2d
Rework ignore mechanism
piegamesde Sep 17, 2022
ff6e33d
Update rfcs/0127-issues-warnings.md
piegamesde Oct 14, 2022
6ffce6d
Update rfcs/0127-issues-warnings.md
piegamesde Oct 14, 2022
24a8985
Remove "resolved" attribute again
piegamesde Oct 15, 2022
1b1424e
Incorporate review feedback
piegamesde Oct 15, 2022
1e6725e
Rewrite (again)
piegamesde Dec 12, 2022
81564e6
Rename throw->error, trace->warn
piegamesde Jan 21, 2023
c5c64ec
Make meta.problems an attrset
piegamesde Jan 31, 2023
2a78241
Rewrite *again*, most change is in the configuration options
piegamesde Apr 28, 2023
4f311de
Review update (WIP)
piegamesde May 15, 2023
b253df1
Review update
piegamesde May 30, 2023
7939f49
Update shepherds list
piegamesde Jun 1, 2023
33ec9a6
Meeting update
piegamesde Jun 19, 2023
83319ec
Typos
piegamesde Jun 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
156 changes: 156 additions & 0 deletions rfcs/0127-issues-warnings.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
---
feature: issues-warnings
start-date: 2022-06-11
author: piegames
co-authors: (find a buddy later to help out with the RFC)
shepherd-team: (names, to be nominated and accepted by RFC steering committee)
shepherd-leader: (name to be appointed by RFC steering committee)
related-issues: https://github.com/NixOS/nixpkgs/pull/177272
---

# RFC: Nixpkgs issues and warnings

## Summary
[summary]: #summary

Introduce an issue system into Nixpkgs, similar to broken and insecure, but with a custom per-package message. This will then be used to warn users about packages that are in need of maintenance. Packages that have an open issue for a long time should eventually be removed.

## Motivation
[motivation]: #motivation

Nixpkgs has the problem that it is often treated as "append-only", i.e. packages only get added but not removed. There are a lot of packages that are broken for a long time, have end-of-life dependencies with known security vulnerabilities or that are otherwise unmaintained.

Let's take the end of life of Python 2 as an example. (This applies to other ecosystems as well, and will come up again and again in the future.) It has sparked a few bulk package removal actions by dedicated persons, but those are pretty work intensive and prone to burn out maintainers. A goal of this RFC is to provide a way to notify all users of a package about the outstanding issues. This will hopefully draw more attention to abandoned packages, and spread the work load. It can also help soften the removal of packages by providing a period for users to migrate away at their own pace.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

Apart from that, there is need for a general per-package warning mechanism in nixpkgs – one that is stronger than `builtins.trace`.
Copy link
Member

Choose a reason for hiding this comment

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

there is

Who needs it and why? What use case is not served by lib.warn?

Copy link
Member

Choose a reason for hiding this comment

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

lib.warn can only be used in a limited fashion due to ofborg's clean eval requirement (no warning messages may be printed when doing the normal eval check iirc). We could of course lift this requirement as an alternative.

Copy link
Member

Choose a reason for hiding this comment

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

(Real) warnings are an important feature, and ofborg must not stop us from using it.

We can make ofborg pass a Nixpkgs config flag to suppress expected warnings. This could be implemented as a list (or nested attrset) of attribute paths that have known issues, that cause ofborg to skip those attributes in its clean eval. aliases.nix can already serve as such a list, combined with a separate list for other issues such as described in this RFC.


## Detailed design
[design]: #detailed-design

### Package issues

A new attribute is added to the `meta` section of a package: `issues`. If present, it is a list of attrsets which each have the following fields:

- `message`: Required. A string message describing the issue with the package.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
- `kind`: Optional but recommended. If present, the resulting warning will be printed as `kind: message`.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
- `date`: Required. An ISO 8601 `yyyy-mm-dd`-formatted date from when the issue was added.
- `urls`: Optional, list of strings. Can be used to link issues, pull requests and other related items.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

Example values:

```nix
meta.issues = [{
kind = "deprecated";
message = "This package depends on Python 2, which has reached end of life.";
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
date = "1970-01-01";
urls = [ "https://github.com/NixOS/nixpkgs/issues/148779" ];
} {
kind = "removal";
message = "The application has been abandoned upstream, use libfoo instead";
date = "1970-01-01";
}];
```

### nixpkgs integration

Two new config options are added to nixpkgs, `ignoreWarningsPredicate` and `ignoreWarningsPackages`. A new environment variable is defined, `NIXPKGS_IGNORE_WARNINGS`. Their semantic and implementation directly parallel the existing "insecure" package handling.

Similarly to broken, insecure and unfree packages, evaluating a package with an issue fails evaluation. Ignoring a package without issues (i.e. they have all been resolved) results in a warning at evaluation time.

## Examples and Interactions
[examples-and-interactions]: #examples-and-interactions

### Package removal

There are two ways issues interact with the removal of packages: Either they get an issue because they are going to be removed, or they are removed because they have an open issue for a prolonged period of time.

- Instead of removing a package directly, it should first get an issue announcing the planned removal. This will allow users to migrate away beforehand. `removal` must be used as `kind` (This will facilitate automation in the future).
Copy link
Member

Choose a reason for hiding this comment

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

Currently we have the informal "mark it as broken, wait, add alias" system. How is this better for that? rg 'broken\s=true' is easy. Would that be an automatically-generated warning? More future potential.. (more room for metadata → r-ryantm style bot that removes packages that have had certain issues added to them → it can also make those issues)

Copy link
Member Author

Choose a reason for hiding this comment

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

That informal system is so informal that I've never heard of it or encountered it in practice, at least not for the use case of removing outdated packages that still technically work otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

I tried to look for references when I posted that but timed out after a minute to continue with the review. Left it here as a future TODO, something for the future work subheading?

- Before branch-off for a new release, all (leaf) packages with issues that predate the previous branch-off are deemed safe for removal (unless stated otherwise). If a package is removed based on its issue, that message becomes part of the new `throw` alias.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

### Propagation across transitive dependencies

When a package that is depended on has an issue, all packages that depend on it will fail to evaluate until that package is ignored or the issue resolved. Sometimes, this is sufficient.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

When the issue requires actions on dependents however, it does not sufficiently inform about all packages that need action. Marking all dependents with that issue is not a good idea either though: it would require users to go through some potentially long dependency chains. Instead, only applications, leaf packages or packages with very few dependents should get the issue.

As an example, take `gksu` with the `gksu` → `libgksu` → `libglade` → `python2` dependency chain (for the sake of the example, ignore that it also depends on EOL Gtk 2). Obviously, `python2` should get an issue. As a leaf/application, `gksu` should get one too (it could be the same, or with an adpated message). For the packages in between, it depends on whether they require individual action or not.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

### Backporting

New issues generally should not be added to stable branches, and also not be backported to them, since this breaks evaluation.

## Drawbacks
[drawbacks]: #drawbacks

- People have voiced strong negative opionions about the prospect of removing packages from nixpkgs at all, especially when they still *technically* work.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
- There is a slight long-term maintenance burden. It is expected to be similar to or slightly greater than the maintenance of our deprecation aliases.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
- Some of the example interactions are built on the premise that nixpkgs is under-maintained, and that most users are at least somewhat involved in the nixpkgs development process. At the time of writing this RFC this is most certainly true, but the effects on this in the future are unknown.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- "in-band" maintenance of this data means that users have to upgrade their nixpkgs to become aware of new issues.
- The proposed approach has significantly less overhead than designing and maintaining a separate database along with tools to combine its data with nixpkgs. We are open to an alternative approach in the future, but do not want to incur such high maintenance costs at this time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Add as sub-point: out of band warnings would be impure if they stopped evaluation like they currently do.

Copy link
Member

Choose a reason for hiding this comment

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

I imagine such a tool not blocking evaluation at all, but providing the metadata, comparing it to local policy, annotating the drvs produced with said metadata, and (when used interactively) providing a "continue anyway" knob if anything happens that's not already permitted by local policy.

I don't count that as an impurity, since I don't want it to be able to modify the evaluation results at all -- only observe evaluation and decide what to do afterwards (which also allows getting all the warnings at once, and displaying all those that local policy considers as "blocking" at once).

## Alternatives
piegamesde marked this conversation as resolved.
Show resolved Hide resolved
[alternatives]: #alternatives

An alternative design would be to have issues as a separate list (not part of the package). Instead of allowing individual packages, one could ignore individual warnings (they'd need an identifying number for that). The advantage of doing this is that one could have one issue and apply it for a lot of packages (e.g. "Python 2 is deprecated"). The main drawback there is that it is more complex.
piegamesde marked this conversation as resolved.
Show resolved Hide resolved

A few other sketches about how the declaration syntax might look like in different scenarios:

```nix
{
# As proposed in the RFC
meta.issues = [{
message = "deprecation: Python 2 is EOL. #12345";
# (Other fields omitted for brevity)
}];

# Issues are defined elsewhere in some nixpkgs-global table, only get referenced in packages
meta.issues = [ "1234-python-deprecation" ];

# Attempt to unify both approaches to allow both ad-hoc and cross-package declaration
meta.issues = {
"1234-python-deprecation" = {
message = "deprecation: Python 2 is deprecated #12345";
};
};
}
```

## Unresolved questions
[unresolved]: #unresolved-questions

- From above: "Ignoring a package without issues (i.e. they have all been resolved) results in a warning at evaluation time". How could this be implemented, and efficiently?
- Should issues be a list or an attrset?
- A lot of bike shedding. (See below)

## Future work
[future]: #future-work

- Issues are designed in a way that they supersede a lot of our "insecure"/"unfree"/"unsupported" packages infrastructure. There is a lot of code duplication between them. In theory, we could migrate some of these to make use of issues. At the very least, we hope that issues are general enough so that no new similar features will have to be added in the future anymore.
- `meta.knownVulnerabilities` is the first candidate to go
- Unfree package handling will probably be out of scope, since we already have some custom filtering based on licences.
- Inspired by the automation of aliases, managing issues can be helped by tooling as well. This is deemed out of scope of this RFC because only real world usage will tell which actions will be worthwhile automating, but it should definitely considered in the future.
- There will likely be need for tooling that lists issues on all nixpkgs packages, filtered by kind or sorted chronologically.
- Automatically removing packages based on time will likely require providing more information whether it is safe to do so or not.
- > If the advisories were a list, and we also added them for modules, maybe we could auto-generate most release notes, and move release notes closer to the code they change. [[source]](https://discourse.nixos.org/t/pre-rfc-package-advisories/19509/4)
- Issues can certainly be automatically integrated into the release notes somehow. However, this alone would not allow us to move most of our release notes into the packages, because for many release entries breaking eval would be overkill.

## Bike shedding
Copy link
Member

Choose a reason for hiding this comment

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

"Flag"?

I don't like that "issue" collides with nixpkgs's github-based issue tracking.

Copy link
Member

Choose a reason for hiding this comment

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

Thinking outside the box here. Perhaps these terms might be useful?

gate, inhibitor, flaw, condition, concern

Copy link
Member Author

Choose a reason for hiding this comment

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

How about calling the nixpkgs config option simply by what it does, maybe something like checkMetaHandler? Because I think this is the only user-facing place where this entire "feature" needs a name. (For the package side, I still think "issues" is the least bad name so far)


Here are a few naming proposals, and how well they would be suited to describe different conditions. "broken" and "unsupported" must be acknowledged but – unlike the others – don't imply some required user action. "unfree" is somewhat out of scope because it is unlikely to be replaced anytime soon.

| Kind | Currently | "Issue" | "Warning" | "Problem" | "Advisory" |
|-------------|-----------|---------|-----------|-----------|------------|
|insecure | `meta.knownVulnerabilities` |✅|✅|✅|✅|
|unmaintained | `meta.maintainers = []` |✅|✅|❓|❓|
|deprecated | n/a |✅|✅|✅|❓|
|removal | n/a |❓|✅|✅|❓|
|||||||
|broken | `meta.broken` |✅|❌|✅|❌|
|unsupported | `meta.platforms` |✅|❓|✅|❌|
|||||||
|unfree | `meta.license` |✅|✅|❌|❓|


"Advisory" was initially chosen based on the notion of security advisories, but was later dismissed as the project grew in scope. "Issue" and "Problem" are similar words, of which the former is a well-known technical term¹ which should be preferred here.

"Issue" and "Warning" are both good candidates, of which the former implies some required action whereas the latter merely wants to inform. In the end, we decided that packages should have *issues* which should produce *warnings* that can be *ignored*. While this distinction may be a bit unintuitive, it will make it easier to generate warnings from things that are not explicitly marked as issues (e.g. missing maintainers).

¹ Non-native speakers: look up the difference between "issue" and "problem" in English :)