Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC 0127] Nixpkgs "problem" infrastructure #127
Changes from 16 commits
60cb23d
ee72999
c109c31
ff2e2d4
f83067a
2daf978
2b63424
e5c0e13
3212d31
ecdbe2c
c907adb
d232a2d
ff6e33d
6ffce6d
24a8985
1b1424e
1e6725e
81564e6
c5c64ec
2a78241
4f311de
b253df1
7939f49
33ec9a6
83319ec
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Who needs it and why? What use case is not served by
lib.warn
?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.
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.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.
(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.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.
I'd argue that scheduled removals should not cause an error like this RFC is seemingly proposing. Or why should they?
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.
Think of it as removing a package by adding a
removal
problem, and then users get some grace period where the package is still available for some time.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.
I don't think packages that have EOL dependencies should be marked as deprecated, they can still be maintained while having EOL dependencies.
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.
The general concept here is correct, although I agree that the name may be misleading. The representative use case for "deprecated" is the Python 2 EOL: some packages can be fixed by upgrading to Python 3 or otherwise removing the Python 2 dependency, others are probably hopeless and doomed.
IMO it's the other way around: packages with EOL dependencies are in need of maintenance
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.
Also, EOL packages receive security fixes or maintenance only via paid subscriptions usually.
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.
(But I'm open to an EOL explicit flag.)
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.
Hello, came to read the rfc after fcp was announced, this is the only thing that bugged me while reading..
It seems there was no conclusion to this comment.
The distinction can be hard to grasp, but I think deprecated and eol/obsolete are clearly different and should be handled differently.
Mozilla's JS docs uses deprecated/obsolete here:
To me (french, not english-native), obsolete also means that something else took its place.
eol
would be stronger and wouldn't mean that.Random guy on english stackexchange says:
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.
@bew, I don't see how this distinction is relevant in this context. The idea behind "deprecation" is, that some package has a problem related to software for which upstream dropped support, and thus needs fixing or removal. Whether or not it currently works is not relevant to me, since this is from a maintainer perspective and not a user perspective.
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.
While I wanted also a distinction, I feel like it's not that important from a maintainer perspective, and I agree with @bew ; though, if, in practice, we found ourselves wanting more, we can always discuss this after extensive experience with that RFC in production.
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.
Ok for the distinction between deprecated/obsolete/eol.
I'm now also thinking about the wording vs semantic difference between
removal
anddeprecated
.Something that is marked for
removal
should not be used anymore, this actually looks like the definition ofdeprecated
🤔Might be confusing, maybe we can rename:
removal
todeprecated
(or don't use rhe worddeprecated
at all?)deprecated
toobsolete
/eol
What do you think? @piegamesde @RaitoBezarius
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.
I'm fine with changing names as long as we keep the originally intended semantics. I'd prefer to keep "removal" because it's for things "that are about to be removed" so I find it quite fitting. "removal" is a least resort measure, and "deprecated" would be too weak of a word for it.
I'd be fine with renaming "deprecated" to "obsolete" or "eol". Personally I don't mind "deprecated" and don't think the alternatives are a big improvement, but they're fine so if people prefer it we can do that.
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.
This feels a bit weird, because the user could declare problems with this kind, but this should never be done, it's entirely automated. Should users maybe not be allowed to declare this problem kind? And should the name
maintainerless
be reserved so there's nobody trying to declare their ownmaintainerless
problem with a different kind? Because if not, what should happen when there's already a problem with that name defined but thenmeta.maintainers
becomes empty? Should problems maybe not be declared automatically at all to make sure there's no name inconsistencies?I feel like this needs a bit more thought.
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.
Good point. How about specifying that some kinds are not allowed for manual declaration in
meta.problems
? I'd nominatemaintainerless
,broken
andunsupported
(the latter two are not currently in use either way, they are just reserved for future work)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.
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)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.
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.
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.
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?
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.
This "unmaintained" boogeyman is a problem endemic to poorly-scoped software. Cough cough browsers cough.
dnscache
,tinydns
, andrunit
have been "unmaintained" for over a decade and work better than most of their competitors. The same is true ofunfs3
, but its alternatives (i.e. in-kernel nfs) can't be run in an unprivileged sandbox, which turns the wholeunmainained==insecure
on its head.Is this RFC going to affect their presence in nixpkgs?
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.
This is not the kind of "unmaintained" the sentence is referring to. Yes, eventually people may start to enforce
maintainers != []
using the features provided by this RFC if they want to, but this will be a new and separate discussion for sure.This sentence is about all a) packages that are effectively unmaintained, i.e. they require some work to keep going but there is nobody and b) packages that are unmaintained or abandoned by upstream with no hope of fixing.
And no, "unmaintained" does not imply it's insecure by any means. Personally, I'd like to get to a point where we treat "end of life" as kind-of insecure. And this is not only about security, but also about workload: I do not want us to patch things on software if it is end of life, instead we should have a graceful way of removing it from
nixpkgs
.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.
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.
Add as sub-point: out of band warnings would be impure if they stopped evaluation like they currently do.
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.
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).
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.
"Flag"?
I don't like that "issue" collides with nixpkgs's github-based issue tracking.
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.
Thinking outside the box here. Perhaps these terms might be useful?
gate, inhibitor, flaw, condition, concern
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.
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)