[RFC 0033] [WIP] Deprecation #33
Conversation
infinisil
changed the title
|
I'll co-author (I'll be fairly busy until Sunday, just so you know). |
|
A few notes in the phone.
The
|
Yeah I have this mentioned in it, I have the intention of doing this as well, but the main focus (for now) is attribute deprecations, as they are most common.
See the first paragraph of the Future work section. That index I'm mentioning there could be including deprecation information of attributes.
Doesn't it do some error handling? Maybe it runs into an abort, which can't be caught, see NixOS/nixpkgs#45637
See https://github.com/Infinisil/rfcs/blob/deprecation/rfcs/0033-deprecation.md#configuration, I have implemented a way to pass config to lib (based on NixOS/nixpkgs#38522), implementation here |
|
@Ericson2314 Oh and thanks for co-authoring! I think @samueldr was also showing interest, but considering he's already release manager he probably has enough to do with that :P |
|
You're right, I was insisting this be contributed, and in another timeline I would probably have looked at co-authoring! Alas, time is a limited resource! |
infinisil
changed the title
|
@edolstra Ideally yes, and I'm excited for having this in the future. Since this will be taking a while though and deprecations are something we'd like to have now, I think this RFC is a good solution. Another problem just discovered, nixpkgs traces apparently break hydra evaluation: NixOS/nixpkgs#46146 (comment). See https://github.com/NixOS/nixpkgs/blob/f38e8c0e96a75862daccc7f978496631dfcf6037/pkgs/top-level/make-tarball.nix#L77-L89 (Thanks for the pointer @srhb) Implementing such a mentioned Nix setting for being quiet and making nix-env use that should fix this problem automatically, this check in nixpkgs can then be removed. The original reason this was added is exactly for this purpose anyways, see commit NixOS/nixpkgs@1357c3d by @dezgeg that introduced this check |
|
What could be done, is the prototype for the user-facing API can be made in nix. Once it feels right, it can be implemented as a language feature, and you get backward compatibility for the current nix versions by keeping the prototype implementation around until |
|
In short: see NixOS/nixpkgs#46530.
I think this a good example of a why technical RFCs are usually useless. Writing my thoughts on how I would go implementing the thing I wrote about below took more time than actually implementing it in the above PR (and then I had to go over my thought and fix them after "runnable code reality check" anyway).
Well, here it goes.
==============
Some comments related to thoughts and SLNOS experience in NixOS/nixpkgs#45717 (comment), in particular
In SLNOS we frequently mark patches as "apply no sooner than <date/release>", i.e. in this case of this PR we would do:
- schedule rename + alias in a week (or apply immediately);
- schedule deprecation warning for the next release;
- schedule deletion for the release after the next release.
In SLNOS we schedule patches (because we can), in this RFC you want to emulate that for attributes via:
- rename + alias immediately;
- add lib.deprecate deprecation warning for the next release immediately;
- add lib.deprecate error for the release after the next release immediately.
Disregarding the discussion on which one is a better method, I think
```nix
{
foo = lib.deprecate {
year = 2018;
month = 8;
attrPath = [ "foo" ];
reason = "Was replaced with bar";
value = "foo";
};
}
```
and its implementation in https://github.com/Infinisil/nixpkgs/tree/deprecation are overly complicated: you spend like 80% of the code there computing dates. What, I think, you really want is simply a smarter "lib.warn", like this:
```nix
foo = lib.deprecate {
silentBefore = "2018.3";
failingAfter = "2018.9";
what = "foo";
reason = "use `bar` instead";
} bar;
bar = value;
```
Implementation then would
- become identity when `lib.versionOlder current silentBefore` (unless you want to see upcoming deprecations),
- become `lib.warn` when `lib.versionOlder silentBefore current && lib.versionOlder current failingAfter`
- become `throw` or `assert` else.
And `current` should default to `lib.trivial.version`, but it should be made configurable via `config`.
Notable differences from the current state of this RFC:
- Both `silentBefore` and `failingAfter` are explicit.
- Comparing versions instead of dates.
As a side effect,
- we wouldn't need `system.nixos.configVersion` of NixOS/nixpkgs#39329 (comment) (the push for which was stopped with reverts) but instead have a repository-wide `lib.configVersion` (or whatever).
I.e.:
- `lib.trivial.version` is the actual version of the thing you run.
- `lib.configVersion` (`<= lib.trivial.version`) is the version you want it feel like.
After that I would implement `mkDeprecatedOptionModule` with the same interface that is either `mkAliasOptionModule`, or `mkRenamedOptionModule`, or an error for NixOS renames.
===========
On lower level, I think, Nixpkgs should simply get log levels, i.e. `lib.log : Level -> Bool -> Message -> a -> a` that inspects `config.traceLevel` and `config.throwLevel` before doing `trace` or `throw`, with over functions implemented in its terms like so:
```
debug = log DEBUG;
info = log INFO; // higher-level `trace`
warn = log WARNING;
fail = log ERROR;
# assert = e: fail (!e); # could be implemented this way, but is a reserved keyword in nix
```
The `lib.deprecate` above then could do `info` instead of being silent, and you wouldn't need a separate option for that. With `config.traceLevel = INFO` you would get upcoming deprecations automatically.
The issue with hydra and warnings is then trivial to solve by setting `config.traceLevel = ERROR;` for hydra.
Btw, if nix would then interpret top-level expressions of the form `<some-keyword> a; b` as `seq a b` like I mentioned in NixOS/nixpkgs#44136 (comment) (or, alternatively, provide something like `($)` operator from Haskell) we could then make the syntax pretty pleasant (without much parentheses) too, e.g.:
```nix
foo = lib.deprecate {
silentBefore = "2018.3";
failingAfter = "2018.9";
reason = "Was replaced with bar";
} $
bar;
assertExample = lib.assert expr message $ expr2;
package = { stdenv, something, else, ... }@Args:
lib.assert (something == false) "Something is broken" $
lib.assert (else == false) "Else is broken" $
stdenv.mkDerivation {
...
};
```
In short, I think without a log level mechanism we would never have a level of warning messages that satisfies everyone. I love warnings, I want warnings about anything if I can be potentially affected by the problem. For me false positives are totally fine if the warning itself makes sense. Meanwhile, @edolstra clearly dislikes false positive warnings. I don't see us ever agreeing on that point.
Thoughts?
|
|
@oxij There appear to be more or less consensus on the $ operator here: NixOS/nix#1845 :) still needs implementation, though. For your prototype of For log functions… well, I think we should discuss that in a separate RFCs, as RFCs where everyone does not agree just never get merged, and the discussion is complex enough as it is :) |
|
@oxij Thanks for the effort! While you have some points, and I admit that I might went overboard a bit, I raise some counterpoints:
Yes indeed, it is a complicated implementation, but ultimately this doesn't matter much. What matters is usability and UX, the end user couldn't care less about code complexity. I'll argue that the notion of "unsupported in releases after deprecation" makes a lot of sense, we want to warn for e.g. one release and generally don't really care about when that release happens. If this were date-based, we'd have to assume that nixpkgs always follows an exact 6-month release cycle, which wasn't always the case in the past and might not be always the case in the future. Have a look at the examples in this section as well. Regarding using a year/date vs a release for specifying the deprecation time: There isn't any difference, both are release-based in the end. I just implemented it with integers because it reduces the number of invalid inputs. I could change that, but then it requires string parsing to get the same features. My implementation also makes sure that every deprecation will fail eventually and the user always informed when that happens. The people adding the deprecation don't have to be bothered adding such a date themselves. That's one thing I find very important, it ensures that deprecation warnings actually have a meaning, users can't ignore them without eventually having to deal with it.
That just fixes a symptom of the problem, the problem is that
I don't think it makes sense to support this:
I like the idea of having different log levels, but I don't think it makes sense to support the So while I like the idea of log levels, the other things you suggested don't sound very thought through. I'm going very much into detail into the desired properties of deprecation in my RFC and make sure that those are consistent and make sense. |
|
Really, deprecation has nothing to do with log levels though, and using log levels to show/hide deprecation warnings is just wrong. You wouldn't call |
|
@infinisil
Firstly, let's note that "18.09", from the point of view of `versionOlder` is, pretty much, just a list of numbers with obvious (categorically free) ordering.
Yes indeed, it is a complicated implementation, but ultimately this doesn't matter much. What matters is usability and UX, the end user couldn't care less about code complexity.
Ugh, I wholeheartedly disagree. What gets no constant KISS-effort either becomes unmanageable and freezes in development or just starts crumbling under its own complexity.
> `lib.configVersion` (`<= lib.trivial.version`) is the version you want it feel like.
I don't think it makes sense to support this:
I think we have to implement `configVersion` regardless of `lib.deprecate`. Many NixOS modules beg for `configVersion`. For instance, we want to change defaults from time to time and we can't disable previously enabled services without silently breaking stuff (like happened with ALSA, for instance, and will happen with initrd networking if no effort for `configVersion` is made). Since `stateVersion` is for a different use case, `configVersion` is the only option I see. The only question is whenever it should be NixOS-specific option or not.
Projecting from this point of view, in my comment above I'm arguing that it makes sense to make `configVersion` Nixpkgs-wide (e.g. we might as well want to change default values of some build options for some packages or something), and hence it makes total sense to tie `lib.deprecate` to `configVersion` (since you have to tie it to something and I see no point in having more knobs, things might get out of sync with more knobs too).
- If one allows the user to emulate the evaluation at a future release, they might make adjustments for planned deprecations,
I see no problem with that.
because it looks like they're final.
How do you define "final"? I see no way to define "final". And if you have some way to define it I'm sure I'll disagree with that definition. If you're renaming things multiple times it's only good manners to always alias all previous versions to the final version. In a good mannered repository I see no problems treating any intermediate state (at merge commits, not inside feature branches) as "final". No issues here.
This would violate the idea of "once you see a deprecation warning, it's sure to be deprecated".
That assumes that we will never have renames `a -> b` followed by `b -> a` in another release. I think that's overly restrictive and also unlikely.
But now I see that we disagree on underlying assumptions about how deprecations are ought to be made in the first place.
I think that attributes should get deprecated without dates initially (i.e. just aliased) and have get their `silentBefore` set only after things get stable enough for a temporary consensus on the resulting names.
For instance, you want to overhaul some subset of NixOS modules, okay, you start your overhaul, make a bunch of PRs, ...
- Yes, please: ... all of which introduce aliases, and then, when everything looks good, you make a final PR with deprecations.
- Please, no: ... all of which introduce deprecations with some dates taken from the top of your head, ...
Users might even think of setting this setting to something far in the future to always tell them all deprecations straight ahead, which would take all the magic out of planned deprecations.
a) It's easy to enforce by adding an `assert (versionOlder config.configVersion trivial.release)` in a well-placed place in my prototype.
b) I'm not sure this restriction makes much sense.
- It doesn't make sense to set this value to something in the past:
What if you have a configuration you want to build on several releases of Nixpkgs (like NixOps does)? Which `configVersion` should you choose? You'd have no other choice but to choose the oldest.
Things in the past might not be there anymore.
If you go far enough back, sure, they might. But I expect to be able to go back several (more than two) releases at the very least.
In principle, if we wish, with proper `configVersion` infrastructure, we can keep being backward-compatible indefinitely (at the cost of linearly growing code bloat).
I'll argue that the notion of "unsupported in <n> releases after deprecation" makes a lot of sense, we want to warn for e.g. one release and generally don't really care about when that release happens.
Where did you get those numbers from? Did I miss a study of publicly available `configuration.nix`es that provides statistics of their use of deprecated options and attributes that shows that some constant time interval works for all possible cases?
I think we SHOULD keep being backward-compatible as long as possible, preferably indefinitely, and in case we can't we MUST specify deprecation timelines explicitly, separately for each thing.
In general, the longer something stayed one way before a change, the longer it would take to deprecate it. E.g. I think it's unlikely we will be able to replace and deprecate the call to `stdenv.mkDerivation` in 15 years if we started today, there's just too much 3rd party stuff that uses it.
If this were date-based, we'd have to assume that nixpkgs always follows an exact 6-month release cycle, which wasn't always the case in the past and might not be always the case in the future.
See the very first sentence of this comment. It's just a version, which is just a list, with obvious ordering. It makes no assumptions about release cycles or anything, as long as it monotonically increases from release to release it will work.
Have a look at the examples in [this section](https://github.com/Infinisil/rfcs/blob/deprecation/rfcs/0033-deprecation.md#deprecation-process) as well.
As noted above, I disagree with the underling assumptions of that section.
Regarding using a year/date vs a release for specifying the deprecation time: There isn't any difference, both are release-based in the end. I just implemented it with integers because it reduces the number of invalid inputs. I could change that, but then it requires string parsing to get the same features.
`versionOlder` does it for you for free.
My implementation also makes sure that every deprecation will fail eventually and the user always informed when that happens. The people adding the deprecation don't have to be bothered adding such a date themselves. That's one thing I find very important, it ensures that deprecation warnings actually have a meaning, users can't ignore them without eventually having to deal with it.
As noted above, I disagree with both statements.
> The issue with hydra and warnings is then trivial to solve by setting `config.traceLevel = ERROR;` for hydra.
That just fixes a symptom of the problem, the problem is that `nix-env -qa` outputs all traces, which is very ugly, see NixOS/nixpkgs@8e8e23de33#commitcomment-12972032. The fact that hydra is failing is just because there is a check in nixpkgs to prevent distributing a tarball that has `nix-env -qa` warnings, I linked it earlier. This problem isn't fixable through nixpkgs.
I see, I saw that patch, but I thought that maybe there was something else that checks instantiation logs on hydra. If there's nothing, then that point of mine can be ignored.
> On lower level, I think, Nixpkgs should simply get log levels, i.e. `lib.log : Level -> Bool -> Message -> a -> a` that inspects `config.traceLevel` and `config.throwLevel` before doing `trace` or `throw`, with over functions implemented in its terms like so:
I like the idea of having different log levels, but I don't think it makes sense to support the `ERROR` level. ...
Yep, that implementation was something I came up in an hour and it looked cool enough to me to include. I will think some more about this later.
I'm going very much into detail into the desired properties of deprecation in my RFC and make sure that those are consistent and make sense.
And I'm advocating "tools, not restrictions" philosophy. People that would do those deprecations would know better than us here arguing from hypothetical examples.
The last thing I want is `lib` enforcing some nonsense bureaucracy restrictions on my code.
|
|
@oxij Sorry for the delay. My short opinions:
I may went a bit overboard with the writing and the implementation though haha, it taught me a lot though, I've never thought so thoroughly about something, and I'll probably use these thoughts in future tools for nixpkgs (see bottom section), there's lots of cool stuff to do. Will make another draft in the future. Might take a bit of time though, there's many things to do that are more interesting than this PR, and my free time is limited :). Regarding NixOS/nixpkgs#46530, I'd appreciate if we could reach consensus here first before intending to make any changes, so it would be nice if you could mark that PR as non-final or something. |
|
- `configVersion`: Good idea for NixOS config, but I don't think it should be used for deprecation
Do you want another knob? Why? Can you elaborate?
- Eventual deprecation: Disagree, this is one of the main points I want deprecation, to be able to eventually remove stuff without annoyed users, removing the cruft
I agree that we probably should remove completely outdated stuff, I'm just saying that with proper `configVersion` tooling we can support it forever if we wish.
Regarding NixOS/nixpkgs#46530, I'd appreciate if we could reach consensus here first before intending to make any changes, so it would be nice if you could mark that PR as non-final or something.
I don't think anyone would merge that seeing the discussion there, but marked [WIP] just in case.
|
I don't think it makes sense to allow this. Let's say that {
foo = lib.deprecate "has been removed";
}Now what should happen if I set So you say "we just don't remove it and keep it there to be backwards compatible", so let's do that: {
foo = lib.deprecate "has been removed" "but here's the value in order to stay backwards compatible";
}So Paul has his
So how about "let's issue a warning only when it has been deprecated for a certain amount of time". Because So you say "Let's make it such that the user can't lag too far behind with their I just don't see how this could work without giving weird and undesired behaviour all around. A solution based on |
|
Firstly, let's assume that nixpkgs will have something like `configVersion` eventually (which, I think, most people here will agree should happen anyway).
By removing `configVersion` from consideration of this RFC (i.e. by disregarding the interactions between `lib.deprecate` and `configVersion`) you don't solve any of the issues you pointed out, you ignore them.
I just don't see how this could work without giving weird and undesired behaviour all around.
Sure, removing old things while also trying to be backwards-compatible is not easy.
I think one sane way to do this is to just limit how far back `configVersion` can go, i.e. `assert (configVersion >= oldestVersionWeCanEmulate)`. Which is similar to one of your suggestions. Producing warnings when `configVersion` gets close to `oldestVersionWeCanEmulate` does seem reasonable, I completely agree.
|
|
I would be willing to be a co-author for this RFC (after it’s been simplified). |
|
Closing all draft RFCs. Please feel free to reopen when this is ready for general community review and the RFC shepherd process. |
|
This comes up again and again, here ofborg and nixos-unstable were broken by We should find a solution. |
|
Hehe, I'm here as well for this RFC 👋. |
Small updates regarding position information
Adds a very powerful function for easy deprecation and potentially a handful of automated tools to support it in the future. Instead of removing an attribute which would result in an uninformative
attribute missingerror for users, one can use said function to deprecate the value instead, which will first warn the user with a message, and in a later release throw an error. It is also possible to define a future deprecation date, up to which point no warning messages will be issued by default, unless the user enables them. Additionally the user will be able to fully control which warnings should get shown. This can be used for the standard library, packages, aliases, functions, package sets, everything in nixpkgs. Here is an example use of the current implementation:Evaluating
foowill evaluate to just"foo"in the 18.03 release, evaluate with a warning in the 18.09 release, and throw an error in any future releases. The warning will look like this:Rendered
main implementation branch
extra implementation branch (see Extra section)
Currently looking for a co-author.@Ericson2314 has volunteered to be co-author \o/