From e5bd4185da7bfe1cb745bcf23db794a704a51d99 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sun, 26 Aug 2018 00:49:58 +0200 Subject: [PATCH 01/24] Initial summary, motivation, design (scope and cases) --- rfcs/0000-deprecation.md | 95 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 rfcs/0000-deprecation.md diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md new file mode 100644 index 000000000..6d28422a2 --- /dev/null +++ b/rfcs/0000-deprecation.md @@ -0,0 +1,95 @@ +--- +feature: deprecation +start-date: 2018-08-25 +author: Silvan Mosberger (@infinisil) +co-authors: (find a buddy later to help our with the RFC) +related-issues: (will contain links to implementation PRs) +--- + +# Summary +[summary]: #summary + +We propose to add a deprecation guideline and a set of accompanying functions to deprecate functionality in nixpkgs. This includes a way to deprecate packages, functions, aliases and function arguments (TODO: Nix version too?). + +# Motivation +[motivation]: #motivation + +Currently nixpkgs doesn't have any standard way to deprecate functionality. When a pinned package version gets too bothersome to keep around, the attribute often just gets dropped (490ca6aa8ae89d0639e1e148774c3cd426fc699a, 28b6f74c3f4402156c6f3730d2ddad8cffcc3445), leaving users with an `attribute missing` error, which is neither helpful nor necessary. Sometimes a more graceful removal is done by replacing the attribute with a `throw ""` (6a458c169b86725b6d0b21918bee4526a9289c2f). This also applies to aliases and function arguments, e.g. 1aaf2be2a022f7cc2dcced364b8725da544f6ff7. TODO: Find more commits that remove aliases/packages/function args. + +These warnings often stay around for a long time and won't get removed until somebody notices them when passing by. + +Why are we doing this? What use cases does it support? What is the expected +outcome? + +# Detailed design +[design]: #detailed-design + +This is the bulk of the RFC. Explain the design in enough detail for somebody +familiar with the ecosystem to understand, and implement. This should get +into specifics and corner-cases, and include examples of how the feature is +used. + +## Backwards Compatibility and its Scope + +The main point of deprecation is to provide a limited amount of backwards compatibility for external code using our codebase when we remove some functionality. An important part to consider is the scope of backwards compatibility, because in theory full backwards compatibilty in every way can only be achieved by not ever changing anything: What if Bob built his code on the assumption that `stringLength (readFile )` would always return 732? Of course nobody will actually do this and we don't need to nor want to keep file lengths constant. Compatibility is therefore always in regards to *how the codebase gets used* in external code. We look at how nixpkgs gets used externally: + +1. Anything accessible through the top-level attribute set `pkgs = import {}` + a. Packages (`pkgs.hello`, `pkgs.gcc`, ...) including their resulting attribute sets + b. The standard library (`pkgs.lib`) + c. Functions (`pkgs.mkDerivation`, `pkgs.fetchzip`, ...) + d. Other package sets (`pkgs.haskellPackages`, `pkgs.pythonPackages`) and their packages and functions +2. The standard library through `import ` +3. NixOS system evaluation `import {}` + a. All NixOS options from modules in `` + b. NixOS profiles in `` (TODO: Couldn't these be implemented as a NixOS option?) + +Point 3a is already dealt with through `mkRemovedOptionModule`, `mkRenamedOptionModule`, etc. and is therefore not in scope for this RFC (TODO: These however don't work in submodules -> options in submodules can't use them). Point 3b is very low-traffic and therefore neither in scope. Point 2 is pretty much the same as 1b. We'll also not distinguish between standard library functions and other functions in `pkgs`. + +We end up with 2 categories and their compatibility properties: + +- Attributes: An existing attribute continues to exist. +- Functions: All accepted function arguments continue to be accepted. + +Note: These apply recursively starting an `pkgs`, and there may be exceptions to this. + +Examples: +- `pkgs.pythonPackages.pytest` continues existing +- `pkgs.fetchurl` continues existing and accepting arguments of the form `{ url = ...; sha256 = ...; }` +- `pkgs.haskellPackages.xmonad.env` continues existing +- A potential exception: `pkgs.fzf.bin` may stop existing in the future and you should rely on `pkgs.lib.getBin` for getting the binary output instead. + +## Cases + +Soft deprecate: Warn first, then throw, then remove. Needs compatible expression. +Hard deprecate: Throw, then remove. Doesn't need compatible expression. +Insta deprecate: Remove immediately. Only use in special occasions. + +An attribute or function argument gets renamed -> Add alias and soft or hard deprecate +An attribute or function argument gets changed in functionality -> Rename it, if possible add functional equivalent alias and soft deprecate, otherwise hard deprecate +An attribute or function argument gets removed -> Soft or hard deprecate + + +# Drawbacks +[drawbacks]: #drawbacks + +Why should we *not* do this? + +# Alternatives +[alternatives]: #alternatives + +What other designs have been considered? What is the impact of not doing this? + +# Unresolved questions +[unresolved]: #unresolved-questions + +What parts of the design are still TBD or unknowns? + +What about errors in library functions? Correct or keep backwards compatible. #41604 + +# Future work +[future]: #future-work + +What future work, if any, would be implied or impacted by this feature +without being directly part of the work? + +From the scope defined above it should be possible to implement a program that creates an index over nixpkgs, containing everything that's intended to be used. This can then be used for automatically verifying backwards compatibility for commits. Also this can be used for providing a tags file to look up functions and attributes. From facbceb6dc7f818ca0b40420f3fcec7fe8c5148b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sun, 26 Aug 2018 22:44:38 +0200 Subject: [PATCH 02/24] properties and types of deprecation, state transitions and messages --- rfcs/0000-deprecation.md | 113 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 107 insertions(+), 6 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 6d28422a2..93c16fabe 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -31,7 +31,7 @@ used. ## Backwards Compatibility and its Scope -The main point of deprecation is to provide a limited amount of backwards compatibility for external code using our codebase when we remove some functionality. An important part to consider is the scope of backwards compatibility, because in theory full backwards compatibilty in every way can only be achieved by not ever changing anything: What if Bob built his code on the assumption that `stringLength (readFile )` would always return 732? Of course nobody will actually do this and we don't need to nor want to keep file lengths constant. Compatibility is therefore always in regards to *how the codebase gets used* in external code. We look at how nixpkgs gets used externally: +The main point of deprecation is to provide a limited amount of backwards compatibility for external code using our codebase when we remove or change some functionality. An important part to consider is the scope of backwards compatibility, because in theory full backwards compatibilty in every way can only be achieved by not ever changing anything: What if Bob built his code on the assumption that `stringLength (readFile )` would always return 732? Of course nobody will actually do this and we don't need to nor want to keep file lengths constant. Compatibility is therefore always in regards to *how the codebase gets used* in external code. We look at how nixpkgs gets used externally: 1. Anything accessible through the top-level attribute set `pkgs = import {}` a. Packages (`pkgs.hello`, `pkgs.gcc`, ...) including their resulting attribute sets @@ -43,7 +43,7 @@ The main point of deprecation is to provide a limited amount of backwards compat a. All NixOS options from modules in `` b. NixOS profiles in `` (TODO: Couldn't these be implemented as a NixOS option?) -Point 3a is already dealt with through `mkRemovedOptionModule`, `mkRenamedOptionModule`, etc. and is therefore not in scope for this RFC (TODO: These however don't work in submodules -> options in submodules can't use them). Point 3b is very low-traffic and therefore neither in scope. Point 2 is pretty much the same as 1b. We'll also not distinguish between standard library functions and other functions in `pkgs`. +Point 3a is already dealt with through `mkRemovedOptionModule`, `mkRenamedOptionModule`, etc. and is therefore not in scope for this RFC (TODO: These however don't work in submodules -> options in submodules can't use them). Point 3b is very low-traffic and therefore neither in scope. Point 2 is pretty much the same as 1b. We'll also not distinguish between standard library functions and other functions in `pkgs`. We also won't be trying to keep expressions backwards compatible in regards to e.g. what elements a list contains. We end up with 2 categories and their compatibility properties: @@ -58,16 +58,116 @@ Examples: - `pkgs.haskellPackages.xmonad.env` continues existing - A potential exception: `pkgs.fzf.bin` may stop existing in the future and you should rely on `pkgs.lib.getBin` for getting the binary output instead. -## Cases +## Implementation + +### Types of deprecation + +Desired deprecation properties + +- User knows deprecation reason +- Old code can be removed +- External code continues to work +- The change can be undone/changed without any UX inconsistencies "predeprecation". Users shouldn't get a warning about deprecation when later a compatibility package will be added to make it work again. + +Types of deprecation + +- Soft: Don't warn at first, then warn, then throw. "We don't know if we really want to deprecate it as of now" +- Firm: First warn, then throw. "We're sure to deprecate it" +- Hard: Throw. "It is deprecated and not supported anymore" +- Instant: Remove code instantly. "It has been deprecated and we want to get the warning out of our codebase" + +Which types have which properties + +| Property | Instant | Hard | Firm | Soft | +| User knows deprecation reason | No | Yes | Yes | Yes | +| Old code can be instantly removed | Yes | Yes | No | No | +| Users expressions continue to evaluate | No | No | Yes | Yes | +| The change can be undone/replaced without UX inconsistencies | No | No | No | Yes | + +## When to deprecate + +- Unsupported (version of a) package + +## Deprecation Timeline + +Because nixpkgs gets a new release every 6 months, it also makes sense to change deprecation behaviour on these boundaries. To implement this, the already existing `` file can be used. + +## Presets + +A: Deprecate with warning now, throw error in next release, remove in distant future +B: Throw error now, remove in distant future +C: Low priority deprecation: Silent warning now, + + +## Allowed State transitions + +firm d val "Been deprecated since d, but your code using it will still work up to including release r(d), but not after that. Do the other thing instead" will change to the hard message when current release > r(d) +hard d "Been deprecated since d, do the other thing instead" + + +Current time is t :: month, next release for a month is r :: month -> release, deprecation time is d :: month -Soft deprecate: Warn first, then throw, then remove. Needs compatible expression. -Hard deprecate: Throw, then remove. Doesn't need compatible expression. -Insta deprecate: Remove immediately. Only use in special occasions. +val -> firm d val, if + - d >= t, can't deprecate in the past +val -> hard d, if + - d == t, can't deprecate in past nor future, because we won't have the value anymore +firm d val -> hard d, if + - r(t) > r(d), we have to wait for the next release to promote a firm to a hard deprecation +firm d val -> removed, if + - d + 24 >= t, a long time has passed +firm d val -> val, if + - d > t, no warnings have been issued before +hard d val -> removed, if + - d + 24 >= t, a long time has passed + + +## Deprecation messages + +Should include +- Date of deprecation +- (optional) Date of expected removal +- Reason +- (optional) Remedy + +## Cases An attribute or function argument gets renamed -> Add alias and soft or hard deprecate An attribute or function argument gets changed in functionality -> Rename it, if possible add functional equivalent alias and soft deprecate, otherwise hard deprecate An attribute or function argument gets removed -> Soft or hard deprecate +nixpkgsVersion -> version. Don't warn by default (but do when user turned on more warnings). + +## Implementation + +Attributes containing the current release year and month, parsed from ``. Deprecation is done like this: +```nix +nix-repl = deprecate.hard 2018 8 "foo has been deprecated in favor of bar"; +nix-repl = deprecate.firm 2018 8 "foo has been deprecated in favor of bar" "foo"; +nix-repl = deprecate.soft 2018 8 "foo has been deprecated in favor of bar" "foo"; +``` + +```nix +fetchGit = { url, sha256 }: "foo"; +fetchGit = { url, sha256 ? null }: "foo"; + +``` + + +## PRs + +https://github.com/NixOS/nixpkgs/pull/32776#pullrequestreview-84012820 + +soft can be the same as firm with an increased deprecation date + +## Todo + +- Option to silence warnings, option to show warnings + +## Examples + +Can be used for deprecating versions when they're not supported anymore by upstream at the exact day. + +Single file with all deprecations as an overlay? Can't implement everything, what about deprecated function args? What about stuff you can't overlay? # Drawbacks [drawbacks]: #drawbacks @@ -85,6 +185,7 @@ What other designs have been considered? What is the impact of not doing this? What parts of the design are still TBD or unknowns? What about errors in library functions? Correct or keep backwards compatible. #41604 +What about function arguments? # Future work [future]: #future-work From af5adc967e72a705e10c121d59a859506c33a90e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 27 Aug 2018 22:42:14 +0200 Subject: [PATCH 03/24] Add some links --- rfcs/0000-deprecation.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 93c16fabe..1bd653a7e 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -157,6 +157,10 @@ fetchGit = { url, sha256 ? null }: "foo"; https://github.com/NixOS/nixpkgs/pull/32776#pullrequestreview-84012820 +https://github.com/NixOS/nixpkgs/issues/18763#issuecomment-406812366 + +https://github.com/NixOS/nixpkgs/issues/22401#issuecomment-277660080 + soft can be the same as firm with an increased deprecation date ## Todo From e77f97d2827d36dc1d23a79d5c9663d609abf3ff Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 29 Aug 2018 01:43:59 +0200 Subject: [PATCH 04/24] clean up, firm -> soft, implementation description --- rfcs/0000-deprecation.md | 121 +++++++++++++++++++++------------------ 1 file changed, 65 insertions(+), 56 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 1bd653a7e..15be9b5e9 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -29,9 +29,11 @@ familiar with the ecosystem to understand, and implement. This should get into specifics and corner-cases, and include examples of how the feature is used. -## Backwards Compatibility and its Scope +## Discussion -The main point of deprecation is to provide a limited amount of backwards compatibility for external code using our codebase when we remove or change some functionality. An important part to consider is the scope of backwards compatibility, because in theory full backwards compatibilty in every way can only be achieved by not ever changing anything: What if Bob built his code on the assumption that `stringLength (readFile )` would always return 732? Of course nobody will actually do this and we don't need to nor want to keep file lengths constant. Compatibility is therefore always in regards to *how the codebase gets used* in external code. We look at how nixpkgs gets used externally: +### Scope + +Backwards compatibility should only concern the intended usage of nixpkgs: 1. Anything accessible through the top-level attribute set `pkgs = import {}` a. Packages (`pkgs.hello`, `pkgs.gcc`, ...) including their resulting attribute sets @@ -43,83 +45,72 @@ The main point of deprecation is to provide a limited amount of backwards compat a. All NixOS options from modules in `` b. NixOS profiles in `` (TODO: Couldn't these be implemented as a NixOS option?) -Point 3a is already dealt with through `mkRemovedOptionModule`, `mkRenamedOptionModule`, etc. and is therefore not in scope for this RFC (TODO: These however don't work in submodules -> options in submodules can't use them). Point 3b is very low-traffic and therefore neither in scope. Point 2 is pretty much the same as 1b. We'll also not distinguish between standard library functions and other functions in `pkgs`. We also won't be trying to keep expressions backwards compatible in regards to e.g. what elements a list contains. +Point 3a is already dealt with through `mkRemovedOptionModule`, `mkRenamedOptionModule`, etc. and is therefore not in scope for this RFC (TODO: These however don't work in submodules -> options in submodules can't use them). Point 3b is very low-traffic and therefore neither in scope (see https://github.com/NixOS/nixpkgs/pull/32776). Point 2 is pretty much the same as 1b. We'll also not distinguish between standard library functions and other functions in `pkgs`. -We end up with 2 categories and their compatibility properties: +We'll therefore limit ourselves to everything accessible through `import {}`. The two main categories and their compatibility properties are: - Attributes: An existing attribute continues to exist. - Functions: All accepted function arguments continue to be accepted. -Note: These apply recursively starting an `pkgs`, and there may be exceptions to this. +Function arguments are secondary to this RFC, as most functions are rarely changed. Therefore attributes and their existence will be the focus. Examples: - `pkgs.pythonPackages.pytest` continues existing -- `pkgs.fetchurl` continues existing and accepting arguments of the form `{ url = ...; sha256 = ...; }` -- `pkgs.haskellPackages.xmonad.env` continues existing +- `pkgs.fetchurl` continues existing - A potential exception: `pkgs.fzf.bin` may stop existing in the future and you should rely on `pkgs.lib.getBin` for getting the binary output instead. -## Implementation - ### Types of deprecation +There can be multiple ways of dealing with deprecation, each having different properties, which we will discuss here. + Desired deprecation properties - User knows deprecation reason - Old code can be removed - External code continues to work -- The change can be undone/changed without any UX inconsistencies "predeprecation". Users shouldn't get a warning about deprecation when later a compatibility package will be added to make it work again. +- (optional) The change can be undone/changed without any UX inconsistencies "predeprecation". Users shouldn't get a warning about deprecation when later a compatibility package will be added to make it work again. Once a warning of deprecation has been issued, it should be deprecated. Types of deprecation -- Soft: Don't warn at first, then warn, then throw. "We don't know if we really want to deprecate it as of now" -- Firm: First warn, then throw. "We're sure to deprecate it" +- Soft(delayed): Don't warn at first, then warn, then throw. "We don't know if we really want to deprecate it as of now" +- Soft: First warn, then throw. "We're sure to deprecate it" - Hard: Throw. "It is deprecated and not supported anymore" - Instant: Remove code instantly. "It has been deprecated and we want to get the warning out of our codebase" Which types have which properties -| Property | Instant | Hard | Firm | Soft | -| User knows deprecation reason | No | Yes | Yes | Yes | -| Old code can be instantly removed | Yes | Yes | No | No | -| Users expressions continue to evaluate | No | No | Yes | Yes | -| The change can be undone/replaced without UX inconsistencies | No | No | No | Yes | - -## When to deprecate - -- Unsupported (version of a) package - -## Deprecation Timeline +| Property | Instant | Hard | Soft | Soft(delayed) | +| User knows deprecation reason | No | Yes | Yes | Yes | +| Old code can be instantly removed | Yes | Yes | No | No | +| Users expressions continue to evaluate | No | No | Yes | Yes | +| The change can be undone/replaced without UX inconsistencies | No | No | No | Yes | -Because nixpkgs gets a new release every 6 months, it also makes sense to change deprecation behaviour on these boundaries. To implement this, the already existing `` file can be used. -## Presets +### Allowed State transitions -A: Deprecate with warning now, throw error in next release, remove in distant future -B: Throw error now, remove in distant future -C: Low priority deprecation: Silent warning now, - - -## Allowed State transitions - -firm d val "Been deprecated since d, but your code using it will still work up to including release r(d), but not after that. Do the other thing instead" will change to the hard message when current release > r(d) +soft d val "Been deprecated since d, but your code using it will still work up to including release r(d), but not after that. Do the other thing instead" will change to the hard message when current release > r(d) hard d "Been deprecated since d, do the other thing instead" Current time is t :: month, next release for a month is r :: month -> release, deprecation time is d :: month -val -> firm d val, if +val -> soft d val, if - d >= t, can't deprecate in the past val -> hard d, if - d == t, can't deprecate in past nor future, because we won't have the value anymore -firm d val -> hard d, if - - r(t) > r(d), we have to wait for the next release to promote a firm to a hard deprecation -firm d val -> removed, if +soft d val -> hard d, if + - r(t) > r(d), we have to wait for the next release to promote a soft to a hard deprecation +soft d val -> removed, if - d + 24 >= t, a long time has passed -firm d val -> val, if +soft d val -> val, if - d > t, no warnings have been issued before hard d val -> removed, if - d + 24 >= t, a long time has passed +### Cases + +- Unsupported/deprecated (version of a) package, like `foobar_0_1` which is for some reason still in the code +- A package gets renamed for consistency reasons ## Deprecation messages @@ -129,29 +120,49 @@ Should include - Reason - (optional) Remedy -## Cases +## Implementation -An attribute or function argument gets renamed -> Add alias and soft or hard deprecate -An attribute or function argument gets changed in functionality -> Rename it, if possible add functional equivalent alias and soft deprecate, otherwise hard deprecate -An attribute or function argument gets removed -> Soft or hard deprecate +### Nix -nixpkgsVersion -> version. Don't warn by default (but do when user turned on more warnings). +For simple cases, there should be a function `lib.deprecate` working like this: +```nix +# Takes current year/month, deprecation reason and the original value +foo = deprecate 2018 8 "Foo has been deprecated" "foo"; +``` -## Implementation +The behaviour of this function should be based on the current release (from ``): +- If the deprecation date is after the current release, just return the value without warning +- If the deprecation date is before the current release but after the last one, emit a warning while evaluating the value +- If the deprecation date is before the current release and before the last one, throw an error + +This corresponds to a soft type deprecation after the given deprecation date for one release, then dropping to a hard deprecation. + +Sometimes more flexibility is needed, for which the following function will get introduced: -Attributes containing the current release year and month, parsed from ``. Deprecation is done like this: ```nix -nix-repl = deprecate.hard 2018 8 "foo has been deprecated in favor of bar"; -nix-repl = deprecate.firm 2018 8 "foo has been deprecated in favor of bar" "foo"; -nix-repl = deprecate.soft 2018 8 "foo has been deprecated in favor of bar" "foo"; +foo = deprecate' { + year = 2018; + month = 8; + removeWarningAfter = 2; # Number of releases to warn for before hard deprecating + reason = "Foo has been deprecated"; +} "foo"; ``` -```nix -fetchGit = { url, sha256 }: "foo"; -fetchGit = { url, sha256 ? null }: "foo"; +The former function can be defined via the latter. -``` +To implement this, a file has to be introduced for tracking the past releases. The choice of only tracking year and month (and not the day) is due to it corresponding to nixpkgs' version numbers and anything more than that seems unnecessary while complicating the implementation (is 2018-09-15 before or after the 18.09 release?). +```nix +{ + releases = [ + { + name = "hummingbird"; + year = 2018; + month = 3; + } + ]; +} +``` ## PRs @@ -161,7 +172,7 @@ https://github.com/NixOS/nixpkgs/issues/18763#issuecomment-406812366 https://github.com/NixOS/nixpkgs/issues/22401#issuecomment-277660080 -soft can be the same as firm with an increased deprecation date +https://github.com/NixOS/nixpkgs/pull/45717 ## Todo @@ -186,8 +197,6 @@ What other designs have been considered? What is the impact of not doing this? # Unresolved questions [unresolved]: #unresolved-questions -What parts of the design are still TBD or unknowns? - What about errors in library functions? Correct or keep backwards compatible. #41604 What about function arguments? @@ -197,4 +206,4 @@ What about function arguments? What future work, if any, would be implied or impacted by this feature without being directly part of the work? -From the scope defined above it should be possible to implement a program that creates an index over nixpkgs, containing everything that's intended to be used. This can then be used for automatically verifying backwards compatibility for commits. Also this can be used for providing a tags file to look up functions and attributes. +From the scope defined above it should be possible to implement a program that creates an index over nixpkgs, containing everything that's intended to be used. This can then be used for automatically verifying backwards compatibility for commits. Also this can be used for providing a tags file to look up functions and attributes. Possibly also for providing an out-of-tree documentation and examples. From c937284205013e562d4031da46f08aaf053fb68c Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 30 Aug 2018 00:38:21 +0200 Subject: [PATCH 05/24] Fix formatting of state transitions --- rfcs/0000-deprecation.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 15be9b5e9..e5653106f 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -96,14 +96,19 @@ Current time is t :: month, next release for a month is r :: month -> release, d val -> soft d val, if - d >= t, can't deprecate in the past + val -> hard d, if - d == t, can't deprecate in past nor future, because we won't have the value anymore + soft d val -> hard d, if - r(t) > r(d), we have to wait for the next release to promote a soft to a hard deprecation + soft d val -> removed, if - d + 24 >= t, a long time has passed + soft d val -> val, if - d > t, no warnings have been issued before + hard d val -> removed, if - d + 24 >= t, a long time has passed From ae093acff2535c2535387d033ff5caa76978726a Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Thu, 30 Aug 2018 00:39:17 +0200 Subject: [PATCH 06/24] Add older deprecation PR --- rfcs/0000-deprecation.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index e5653106f..c909962f0 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -179,6 +179,8 @@ https://github.com/NixOS/nixpkgs/issues/22401#issuecomment-277660080 https://github.com/NixOS/nixpkgs/pull/45717 +https://github.com/NixOS/nixpkgs/pull/19315 + ## Todo - Option to silence warnings, option to show warnings From e4739feb4e160a941c7d77a380ad53752046498e Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 4 Sep 2018 20:51:24 +0200 Subject: [PATCH 07/24] Update summary and motivation --- rfcs/0000-deprecation.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index c909962f0..56c2e0b78 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -9,14 +9,12 @@ related-issues: (will contain links to implementation PRs) # Summary [summary]: #summary -We propose to add a deprecation guideline and a set of accompanying functions to deprecate functionality in nixpkgs. This includes a way to deprecate packages, functions, aliases and function arguments (TODO: Nix version too?). +We propose to add a deprecation guideline and a set of accompanying functions and tools to deprecate functionality in nixpkgs. Instead of removing an attribute which would result in an `attribute missing` error for users, one can deprecate a function, which will then first throw a warning and later throw an error. This is intended to be used for anything accessible from `import {}`, including the standard library, packages, functions, aliases, package sets, etc. # Motivation [motivation]: #motivation -Currently nixpkgs doesn't have any standard way to deprecate functionality. When a pinned package version gets too bothersome to keep around, the attribute often just gets dropped (490ca6aa8ae89d0639e1e148774c3cd426fc699a, 28b6f74c3f4402156c6f3730d2ddad8cffcc3445), leaving users with an `attribute missing` error, which is neither helpful nor necessary. Sometimes a more graceful removal is done by replacing the attribute with a `throw ""` (6a458c169b86725b6d0b21918bee4526a9289c2f). This also applies to aliases and function arguments, e.g. 1aaf2be2a022f7cc2dcced364b8725da544f6ff7. TODO: Find more commits that remove aliases/packages/function args. - -These warnings often stay around for a long time and won't get removed until somebody notices them when passing by. +Currently nixpkgs doesn't have any standard way to deprecate functionality. When something is too bothersome to keep around, the attribute often just gets dropped (490ca6aa8ae89d0639e1e148774c3cd426fc699a, 28b6f74c3f4402156c6f3730d2ddad8cffcc3445), leaving users with an `attribute missing` error, which is neither helpful nor necessary, because Nix has functionality for warning the user of deprecation or throwing useful error messages. This approach has been used before, but on a case-by-case basis (6a458c169b86725b6d0b21918bee4526a9289c2f). (Todo: link to 1aaf2be2a022f7cc2dcced364b8725da544f6ff7 and find more commits that remove aliases/packages/function args). Why are we doing this? What use cases does it support? What is the expected outcome? From 854dae2ed736881a722e4ba3eeb0200f4f13bda6 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Tue, 4 Sep 2018 22:21:39 +0200 Subject: [PATCH 08/24] Refine scope, rework state transitions and primitives for them --- rfcs/0000-deprecation.md | 81 +++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 38 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 56c2e0b78..abb834616 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -50,7 +50,7 @@ We'll therefore limit ourselves to everything accessible through `import r(d) -hard d "Been deprecated since d, do the other thing instead" - - -Current time is t :: month, next release for a month is r :: month -> release, deprecation time is d :: month - -val -> soft d val, if - - d >= t, can't deprecate in the past - -val -> hard d, if - - d == t, can't deprecate in past nor future, because we won't have the value anymore - -soft d val -> hard d, if - - r(t) > r(d), we have to wait for the next release to promote a soft to a hard deprecation - -soft d val -> removed, if - - d + 24 >= t, a long time has passed - -soft d val -> val, if - - d > t, no warnings have been issued before - -hard d val -> removed, if - - d + 24 >= t, a long time has passed +| Property | Removal | Throw | Warn | Warn(delayed) | +| User knows deprecation reason | No | Yes | Yes | Yes | +| Old code can be instantly removed | Yes | Yes | No | No | +| Users expressions continue to evaluate | No | No | Yes | Yes | +| The change can be undone/replaced without UX inconsistencies | No | No | No | Yes | + +#### State transitions + +The properties of the four deprecation types along with the properties they ensure lead us to a set of allowed transitions between them. We will use the following primitives to represent deprecation. Here `r` represents the current release as an integer. + +- `val` signifies a non-deprecated value `val`. In Nix this corresponds to `val` itself +- `warn d n val` signifies deprecation since release `d` and unsupported since release `d + n`. In Nix this corresponds to a function like this: + ```nix + if r < d then val # The deprecation time is the future, don't emit any message right now + else if r >= d + n then throw "Deprecated since ${d}, removed in ${d + n}" # The current time is after the time of the planned throw + else builtins.trace "Deprecated since ${d}, will be removed in ${d + n}" val` # The deprecation time is not in the future and before the planned throw + ``` +- `throw d n` signifies a deprecation since release `d` and unsupported since release `d + n`, the same as `warn` without a `val`. In Nix this corresponds to `throw "Deprecated since ${d}, removed in ${d + n}`. +- `removed`, the attribute is removed. In Nix this corresponds to an `attribute missing` error. + +Our deprecation types map to these primitives like this: + +- Warn(delayed) is `warn d n val` with `r < d`, aka deprecate in the future +- Warn is `warn d n val` with `r == d`, aka deprecate now +- Throw is `throw d n` +- Removal is `removed` + +This leads us to the following allowed state transitions between our primitives: + +- `val` -> `warn d n val`, if `r <= d`, aka can't deprecate in the past +- `warn d n val` -> `val`, if `r < d`, aka no warning has been issued yet, we can safely remove the deprecation again without causing any inconsistencies +- `val` -> `throw d n`, if `d == r`, aka can't deprecate in past nor future, because we won't have the value anymore +- `warn d n val` -> `throw d n`, if `r >= d -> r >= d + n`, aka if a warning has been issued, we need to wait until the warning time has passed to transition to a throw. This is the same as just `r >= d + n` +- `warn d n val` -> `removed`, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. +- `throw d n` -> removed, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. ### Cases From 644bfc5780f03a7a864c79a08f0539e3ccdc7435 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 00:21:24 +0200 Subject: [PATCH 09/24] Detailed but untested implementation --- rfcs/0000-deprecation.md | 141 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 131 insertions(+), 10 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index abb834616..0267bed49 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -115,20 +115,126 @@ This leads us to the following allowed state transitions between our primitives: - `warn d n val` -> `removed`, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. - `throw d n` -> removed, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. -### Cases +## Implementation -- Unsupported/deprecated (version of a) package, like `foobar_0_1` which is for some reason still in the code -- A package gets renamed for consistency reasons +### Release tracking -## Deprecation messages +To implement deprecation functions that can vary their behaviour depending on the number of releases that have passed since initial deprecation, we need to track past releases. A file `` should be introduced with contents of the form +```nix +map ({ release, ... }@attrs: attrs // { + yearMonth = let + year = lib.toInt (lib.versions.major release); + month = lib.toInt (lib.versions.minor release); + in (2000 + year) * 12 + month; +}) [ + { + name = "Impala"; + release = "18.03"; + year = 2018; + month = 4; + day = 4; + } + { + name = "Hummingbird"; + release = "17.09"; + year = 2017; + month = 9; + day = ??; + } + { + name = "Gorilla"; + release = "17.03"; + year = 2017; + month = 3; + day = 31; + } + # ... +] +``` -Should include -- Date of deprecation -- (optional) Date of expected removal -- Reason -- (optional) Remedy +Additional fields can be added if the need arises. This file should be updated at date of release. It may also be updated already at branch-off time (TODO: Think about this some more). -## Implementation +### Deprecation functions + +A function `lib.deprecate` will be introduced. When an attribute should be deprecated, it can be used like follows: +```nix +{ + foo = lib.deprecate' { + year = 2018; + month = 8; + warnFor = 2; + reason = "Use bar instead"; + value = "foo"; + }; +} +``` + +Optionally, for convenience, a shorthand `lib.deprecate'` can be provided as well: +```nix +{ + bar = lib.deprecate 2018 8 "Bar is a burden to the codebase" "bar"; +} +``` + +Which will correspond to +```nix +{ + bar = lib.deprecate' { + year = 2018; + month = 8; + warnFor = 1; + reason = "Bar is a burden to the codebase"; + value = "bar"; + }; +} +``` + +The basic implementation and meaning of arguments is a follows (Note: I didn't test this yet). +```nix +{ + releaseYearMonth = let + year = lib.toInt (lib.versions.major lib.trivial.release); + month = lib.toInt (lib.versions.minor lib.trivial.release); + in (2000 + year) * 12 + month; + + allReleases = map (r: r.yearMonth) releases ++ [ releaseYearMonth ]; + + takeWhile = pred: list: ...; + + deprecate' = + { year # Year of deprecation as an integer + , month # Month of deprecation as an integer + , warnFor ? 1 # Number of releases to warn before throwing + , reason # Reason of deprecation + , value ? null # The value to evaluate to, may be null if the current release throws + }: + if warnFor < 0 then abort "the warnFor argument to lib.deprecate needs to be non-negative" else + let + deprecationYearMonth = year * 12 + month; + depReleases = takeWhile (ym: ym > deprecationYearMonth) allReleases; + + isDeprecated = depReleases != []; + deprecationRelease = if isDeprecated then last depReleases else + releaseYearMonth + ((deprecationYearMonth - releaseYearMonth) / 6) * 6); + isRemoved = depReleases > warnFor; + removedRelease = if isRemoved then elemAt depReleases (length depReleases - warnFor) else + releaseYearMonth + ((deprecationYearMonth - releaseYearMonth) / 6 + warnFor) * 6); + prettyRelease = yearMonth: "${toString (div yearMonth 12 - 2000)}.${toString (mod yearMonth 12)}"; + deprecationString = if isDeprecated then "Deprecated since ${prettyRelease deprecationRelease}" else + "Will be deprecated in ${prettyRelease deprecationRelease}"; + removedString = if isRemoved then ", removed since ${prettyRelease removedRelease}" else + ", will be removed in ${prettyRelease removedRelease}"; + message = "${deprecationString}${removedString}. Reason: ${reason}"; + in + if ! isDeprecated then assert value != null; value + else if isRemoved then throw removedString + else assert value != null; builtins.trace deprecationString value; + + deprecate = year: month: reason: value: deprecate { + inherit year month reason value; + }; +} +``` ### Nix @@ -172,6 +278,19 @@ To implement this, a file has to be introduced for tracking the past releases. T } ``` +### Cases + +- Unsupported/deprecated (version of a) package, like `foobar_0_1` which is for some reason still in the code +- A package gets renamed for consistency reasons + +## Deprecation messages + +Should include +- Date of deprecation +- (optional) Date of expected removal +- Reason +- (optional) Remedy + ## PRs https://github.com/NixOS/nixpkgs/pull/32776#pullrequestreview-84012820 @@ -184,6 +303,8 @@ https://github.com/NixOS/nixpkgs/pull/45717 https://github.com/NixOS/nixpkgs/pull/19315 +https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-418424080 + ## Todo - Option to silence warnings, option to show warnings From 81096b6a06acccf75a9774de867c6633a6810247 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 00:23:37 +0200 Subject: [PATCH 10/24] Fix allReleases order --- rfcs/0000-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 0267bed49..596b4b0bd 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -197,7 +197,7 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t month = lib.toInt (lib.versions.minor lib.trivial.release); in (2000 + year) * 12 + month; - allReleases = map (r: r.yearMonth) releases ++ [ releaseYearMonth ]; + allReleases = [ releaseYearMonth ] ++ map (r: r.yearMonth) releases; takeWhile = pred: list: ...; From 15acec546ed04cdb818a31005417d32437a690e1 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 00:34:19 +0200 Subject: [PATCH 11/24] Fix and refine implementation --- rfcs/0000-deprecation.md | 58 +++++++--------------------------------- 1 file changed, 9 insertions(+), 49 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 596b4b0bd..022e04fef 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -207,11 +207,13 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t , warnFor ? 1 # Number of releases to warn before throwing , reason # Reason of deprecation , value ? null # The value to evaluate to, may be null if the current release throws - }: + }@args: if warnFor < 0 then abort "the warnFor argument to lib.deprecate needs to be non-negative" else let deprecationYearMonth = year * 12 + month; depReleases = takeWhile (ym: ym > deprecationYearMonth) allReleases; + location = let pos = unsafeGetAttrPos "reason" args; in + pos.file + ":" + toString pos.line; isDeprecated = depReleases != []; deprecationRelease = if isDeprecated then last depReleases else @@ -224,59 +226,17 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t "Will be deprecated in ${prettyRelease deprecationRelease}"; removedString = if isRemoved then ", removed since ${prettyRelease removedRelease}" else ", will be removed in ${prettyRelease removedRelease}"; - message = "${deprecationString}${removedString}. Reason: ${reason}"; + message = "${deprecationString}${removedString} at ${location}. ${reason}"; in - if ! isDeprecated then assert value != null; value - else if isRemoved then throw removedString - else assert value != null; builtins.trace deprecationString value; - - deprecate = year: month: reason: value: deprecate { - inherit year month reason value; - }; + if ! isDeprecated then assert value != null; value # TODO: Colors and stuff? + else if isRemoved then throw message + else assert value != null; builtins.trace message value; } ``` -### Nix - -For simple cases, there should be a function `lib.deprecate` working like this: -```nix -# Takes current year/month, deprecation reason and the original value -foo = deprecate 2018 8 "Foo has been deprecated" "foo"; -``` +### Configuration -The behaviour of this function should be based on the current release (from ``): -- If the deprecation date is after the current release, just return the value without warning -- If the deprecation date is before the current release but after the last one, emit a warning while evaluating the value -- If the deprecation date is before the current release and before the last one, throw an error - -This corresponds to a soft type deprecation after the given deprecation date for one release, then dropping to a hard deprecation. - -Sometimes more flexibility is needed, for which the following function will get introduced: - -```nix -foo = deprecate' { - year = 2018; - month = 8; - removeWarningAfter = 2; # Number of releases to warn for before hard deprecating - reason = "Foo has been deprecated"; -} "foo"; -``` - -The former function can be defined via the latter. - -To implement this, a file has to be introduced for tracking the past releases. The choice of only tracking year and month (and not the day) is due to it corresponding to nixpkgs' version numbers and anything more than that seems unnecessary while complicating the implementation (is 2018-09-15 before or after the 18.09 release?). - -```nix -{ - releases = [ - { - name = "hummingbird"; - year = 2018; - month = 3; - } - ]; -} -``` +Sometimes more one might wish to either silence certain warnings or ### Cases From 098c41cf32a7c3093aa0035e128127de8a32edfe Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 01:11:00 +0200 Subject: [PATCH 12/24] Explain year/month, add configuration section --- rfcs/0000-deprecation.md | 48 +++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 13 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 022e04fef..50fbbaa8c 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -117,15 +117,19 @@ This leads us to the following allowed state transitions between our primitives: ## Implementation +The implementation of such a deprecation function will purely based on the planned release version of the next release (e.g. 18.09) and release versions of past releases. An argument could then be made that deprecation dates should also just be a release version, however it appears to be more convenient and less error prone if we allow all year/month combinations. Deprecations can then be added simply by inserting the current or planned deprecation year and month, instead of having to figure out what the next (or later than next) release version is going to be. This will work correctly as long as the `` file always contains a version number that's not in the past. This can indeed occur if the date of effective release stretches itself past the month of the release version, which isn't a big problem however, even if unnoticed (the deprecation will just be delayed by a release). + +To make implementation more efficient and easier, a pair of release version year/month will be converted into an integer corresponding to the number of months since year 2000. This means the release version `"18.09"` will be handled as `18 * 12 + 9 = 225`. In the implementation variables that contain such a value will be named `...YearMonth`. + ### Release tracking -To implement deprecation functions that can vary their behaviour depending on the number of releases that have passed since initial deprecation, we need to track past releases. A file `` should be introduced with contents of the form +To implement deprecation functions that can vary their behavior depending on the number of releases that have passed since initial deprecation, we need to track past releases. A file `` should be introduced with contents of the form ```nix map ({ release, ... }@attrs: attrs // { yearMonth = let year = lib.toInt (lib.versions.major release); month = lib.toInt (lib.versions.minor release); - in (2000 + year) * 12 + month; + in year * 12 + month; }) [ { name = "Impala"; @@ -154,7 +158,7 @@ map ({ release, ... }@attrs: attrs // { Additional fields can be added if the need arises. This file should be updated at date of release. It may also be updated already at branch-off time (TODO: Think about this some more). -### Deprecation functions +### Deprecation function A function `lib.deprecate` will be introduced. When an attribute should be deprecated, it can be used like follows: ```nix @@ -163,27 +167,27 @@ A function `lib.deprecate` will be introduced. When an attribute should be depre year = 2018; month = 8; warnFor = 2; - reason = "Use bar instead"; + reason = "foo has been deprecated, use bar instead"; value = "foo"; }; } ``` -Optionally, for convenience, a shorthand `lib.deprecate'` can be provided as well: +Optionally, for convenience, a shorthand `lib.deprecate'` could be provided as well: ```nix { - bar = lib.deprecate 2018 8 "Bar is a burden to the codebase" "bar"; + bar = lib.deprecate' 2018 8 "bar is a burden to the codebase, refrain from using it" "bar"; } ``` Which will correspond to ```nix { - bar = lib.deprecate' { + bar = lib.deprecate { year = 2018; month = 8; warnFor = 1; - reason = "Bar is a burden to the codebase"; + reason = "bar is burden to the codebase, refrain from using it"; value = "bar"; }; } @@ -195,13 +199,13 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t releaseYearMonth = let year = lib.toInt (lib.versions.major lib.trivial.release); month = lib.toInt (lib.versions.minor lib.trivial.release); - in (2000 + year) * 12 + month; + in year * 12 + month; allReleases = [ releaseYearMonth ] ++ map (r: r.yearMonth) releases; takeWhile = pred: list: ...; - deprecate' = + deprecate = { year # Year of deprecation as an integer , month # Month of deprecation as an integer , warnFor ? 1 # Number of releases to warn before throwing @@ -210,7 +214,7 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t }@args: if warnFor < 0 then abort "the warnFor argument to lib.deprecate needs to be non-negative" else let - deprecationYearMonth = year * 12 + month; + deprecationYearMonth = (year - 2000) * 12 + month; depReleases = takeWhile (ym: ym > deprecationYearMonth) allReleases; location = let pos = unsafeGetAttrPos "reason" args; in pos.file + ":" + toString pos.line; @@ -221,7 +225,7 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t isRemoved = depReleases > warnFor; removedRelease = if isRemoved then elemAt depReleases (length depReleases - warnFor) else releaseYearMonth + ((deprecationYearMonth - releaseYearMonth) / 6 + warnFor) * 6); - prettyRelease = yearMonth: "${toString (div yearMonth 12 - 2000)}.${toString (mod yearMonth 12)}"; + prettyRelease = yearMonth: "${toString (div yearMonth 12)}.${toString (mod yearMonth 12)}"; deprecationString = if isDeprecated then "Deprecated since ${prettyRelease deprecationRelease}" else "Will be deprecated in ${prettyRelease deprecationRelease}"; removedString = if isRemoved then ", removed since ${prettyRelease removedRelease}" else @@ -236,7 +240,24 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t ### Configuration -Sometimes more one might wish to either silence certain warnings or +Sometimes one might wish to either silence certain warnings or turn on warnings for planned deprecations. To do this, the nixpkgs `config` attribute will be used: +```nix +import { + config.deprecation = { + warnPlannedDeprecations = true; + warnOverrides = { + foo.bar = false; # Turn off warning for foo.bar + qux.florp = true; # Turn on warning for qux.florp if it's a planned warning for now + }; + }; +} +``` + +TODO: Could there warning overrides be used for more than just deprecations? + +Implementation details: Add a `config` attribute to `lib` with the value `{}` and use it for the deprecation function. This value can be changed by using `lib.extends (self: super: { config = ; } }`. Define `pkgs.lib` as `lib.extends (self: super: { config = config.deprecation; }`. + +## ### Cases @@ -268,6 +289,7 @@ https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-418424080 ## Todo - Option to silence warnings, option to show warnings +- Introduce automatic capture of attribute path in nixpkgs for better error messages ## Examples From 693f5394a969fd817222a4c7608038fbe2fcee2b Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 04:55:00 +0200 Subject: [PATCH 13/24] add deprecation process section with alias statistics --- rfcs/0000-deprecation.md | 41 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 40 insertions(+), 1 deletion(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 50fbbaa8c..5de2bce3f 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -232,6 +232,7 @@ The basic implementation and meaning of arguments is a follows (Note: I didn't t ", will be removed in ${prettyRelease removedRelease}"; message = "${deprecationString}${removedString} at ${location}. ${reason}"; in + # TODO: Improve message for warnFor = 0 if ! isDeprecated then assert value != null; value # TODO: Colors and stuff? else if isRemoved then throw message else assert value != null; builtins.trace message value; @@ -257,7 +258,45 @@ TODO: Could there warning overrides be used for more than just deprecations? Implementation details: Add a `config` attribute to `lib` with the value `{}` and use it for the deprecation function. This value can be changed by using `lib.extends (self: super: { config = ; } }`. Define `pkgs.lib` as `lib.extends (self: super: { config = config.deprecation; }`. -## +## Deprecation process + +When a decision has been made to deprecate an attribute, one has to decide: +- Date of deprecation. Let's call it `d` here, meaning "deprecate after `d` time has passed from now on". This corresponds to the `year` and `month` arguments to the deprecation function. +- The number of releases it should still be evaluable for albeit with a warning, let's call it `w` here, meaning it should warn for `w` releases. This corresponds to the `warnFor` argument of the deprecation function. + +A sane default is `d = 0, w = 1`, to deprecate immediately and warn for one release. In general, it depends on the situation. Some more interesting examples might include: + +- Pinning an old release of a package `hello_0_1` could have a planned deprecation of 2 years after introduction (`d = 2 years`) such that it can be removed eventually. +- A package being unable to be built anymore because it would require big changes might be deprecated immediately with no warning phase (`d = 0, w = 0`), such that the value doesn't have to be kept around anymore. +- A change that will influence a lot of code and might be hard to migrate from could have an instant deprecation but a warning phase of 4 releases (`d = 0, w = 4`), such that people have a lot of time to migrate away from it. +- Aliases might be deprecated only after two years so people enabling all warnings can be alarmed of the eventual removal. The warning phase could then be about 4 releases long because aliases don't cost much anyways (`d = 2 years, w = 4`). + +This isn't main focus of this RFC, however we will address some issues regarding this. + +### Aliases + +Aliases have a long history of being introduced due to either the old name being unfit, the attribute being moved to a package set (or moved from one) or naming convention changes (`_` vs `-`). The question here is: Is it even worth deprecating the old aliases? We look at advantages and disadvantages: + +Advantages of removing aliases: +1. Nix evaluation will be faster and use less memory for everybody. + Doing a quick analysis using `export NIX_SHOW_STATS=1` on nixpkgs de825a4eaacd and Nix 2.0.4 with either all or (almost) no aliases with [this diff](https://gist.github.com/7f647d15b27825c3cc4dc2079acddb39), these are the results for different commands executed in the nixpkgs root. Time has been measured with an initial unmeasured run and an average over multiple runs after that, standard derivation in parens, in seconds. Heap is deterministic. + + | Command | time elapsed (aliases -> no aliases) | total Boehm heap allocations (aliases -> no aliases) | + | `nix-instantiate --eval -A path`| (15 runs), 0.043 (0.0041) -> 0.042 (0.0042) | 4.12MB -> 3.89MB | + | `nix-instantiate -A openssl`| (15 runs) 0.095 (0.0060) -> 0.090 (0.0049) | 18.5MB -> 17.7MB | + | `nix-instantiate nixos/release.nix -A iso_graphical` | (15 runs) 5.71 (0.19) -> 5.64 (0.10) | 839MB -> 836MB | + | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | + + Putting some statistical meaning into the timings using an alpha of 2.5% (z = 1.96) leads to a result of everything but the first command being statistically significant (maybe because it's the only --eval?). So we really got ourselves about 1-3% better speed, which is not much, but it's not nothing. Also 0.4-5% less memory consumption, depending on the expression. + +2. Cruft can be removed, cleaner repository state. + +Disadvantages of removing aliases: +1. Deprecation needed -> People will eventually see warnings, potentially lots of them + Doing a quick analysis of alias introduction times using `cat pkgs/top-level/aliases.nix | rg '[0-9]+-[0-9]+-[0-9]+' -o | cut -d- -f-2 | sort | uniq -c` + +Assigning weights + ### Cases From 4fb1b93d81ccd33baea0260ab6ae65864c3c67c0 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 05:13:46 +0200 Subject: [PATCH 14/24] Analyze alias deprecation --- rfcs/0000-deprecation.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 5de2bce3f..94966e5c1 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -288,14 +288,13 @@ Advantages of removing aliases: | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | Putting some statistical meaning into the timings using an alpha of 2.5% (z = 1.96) leads to a result of everything but the first command being statistically significant (maybe because it's the only --eval?). So we really got ourselves about 1-3% better speed, which is not much, but it's not nothing. Also 0.4-5% less memory consumption, depending on the expression. - -2. Cruft can be removed, cleaner repository state. +2. Cruft can be removed, cleaner repository state. Not that significant, since aliases are in their own file in `` and are therefore very isolated from everything else. Disadvantages of removing aliases: 1. Deprecation needed -> People will eventually see warnings, potentially lots of them - Doing a quick analysis of alias introduction times using `cat pkgs/top-level/aliases.nix | rg '[0-9]+-[0-9]+-[0-9]+' -o | cut -d- -f-2 | sort | uniq -c` + Doing a quick analysis of alias introduction times using `cat pkgs/top-level/aliases.nix | rg '[0-9]+-[0-9]+-[0-9]+' -o | cut -d- -f-2 | sort | uniq -c`, we get an mean of 5.5 and a median of 3 new aliases per month over the last ~4 years. This means if aliases were to be deprecated at a constant rate, we'd see about 5 new deprecations every month, so about 30 every release. Now of course not everybody uses every nixpkgs attribute, so it will be less than that for individual users. Considering that aliases will only get introduced for popular packages (since unpopular ones very little people use to begin with, so aliases wouldn't get introduced a lot), I estimate that an average user will encounter about 0-2 new alias deprecations per release. This amount will of course be bigger with "power" users, using lots of different packages. Considering that there are quite a few people only upgrading their version every 1-2 years, this can add up. -Assigning weights +Assigning weights to ### Cases From 75c27fbd2de1ff220ed9646136e8957fd3ab3a80 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 05:16:48 +0200 Subject: [PATCH 15/24] Properly format tables, maybe --- rfcs/0000-deprecation.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 94966e5c1..fb14eab22 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -80,6 +80,7 @@ Deprecation happens in a sequence of phases, which will correspond to the releas Which types have which properties | Property | Removal | Throw | Warn | Warn(delayed) | +| --- | --- | --- | --- | --- | | User knows deprecation reason | No | Yes | Yes | Yes | | Old code can be instantly removed | Yes | Yes | No | No | | Users expressions continue to evaluate | No | No | Yes | Yes | @@ -281,11 +282,12 @@ Advantages of removing aliases: 1. Nix evaluation will be faster and use less memory for everybody. Doing a quick analysis using `export NIX_SHOW_STATS=1` on nixpkgs de825a4eaacd and Nix 2.0.4 with either all or (almost) no aliases with [this diff](https://gist.github.com/7f647d15b27825c3cc4dc2079acddb39), these are the results for different commands executed in the nixpkgs root. Time has been measured with an initial unmeasured run and an average over multiple runs after that, standard derivation in parens, in seconds. Heap is deterministic. - | Command | time elapsed (aliases -> no aliases) | total Boehm heap allocations (aliases -> no aliases) | - | `nix-instantiate --eval -A path`| (15 runs), 0.043 (0.0041) -> 0.042 (0.0042) | 4.12MB -> 3.89MB | - | `nix-instantiate -A openssl`| (15 runs) 0.095 (0.0060) -> 0.090 (0.0049) | 18.5MB -> 17.7MB | - | `nix-instantiate nixos/release.nix -A iso_graphical` | (15 runs) 5.71 (0.19) -> 5.64 (0.10) | 839MB -> 836MB | - | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | + | Command | time elapsed (aliases -> no aliases) | total Boehm heap allocations (aliases -> no aliases) | + | --- | --- | --- | + | `nix-instantiate --eval -A path` | (15 runs), 0.043 (0.0041) -> 0.042 (0.0042) | 4.12MB -> 3.89MB | + | `nix-instantiate -A openssl` | (15 runs) 0.095 (0.0060) -> 0.090 (0.0049) | 18.5MB -> 17.7MB | + | `nix-instantiate nixos/release.nix -A iso_graphical` | (15 runs) 5.71 (0.19) -> 5.64 (0.10) | 839MB -> 836MB | + | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | Putting some statistical meaning into the timings using an alpha of 2.5% (z = 1.96) leads to a result of everything but the first command being statistically significant (maybe because it's the only --eval?). So we really got ourselves about 1-3% better speed, which is not much, but it's not nothing. Also 0.4-5% less memory consumption, depending on the expression. 2. Cruft can be removed, cleaner repository state. Not that significant, since aliases are in their own file in `` and are therefore very isolated from everything else. From 6bd5f30d1e75f5dbfe6001ad1995dd0d8f4df430 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 05:51:56 +0200 Subject: [PATCH 16/24] fix some stuff, expand future work, add drawbacks --- rfcs/0000-deprecation.md | 53 ++++++++++++++-------------------------- 1 file changed, 19 insertions(+), 34 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index fb14eab22..d425a3059 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -164,7 +164,7 @@ Additional fields can be added if the need arises. This file should be updated a A function `lib.deprecate` will be introduced. When an attribute should be deprecated, it can be used like follows: ```nix { - foo = lib.deprecate' { + foo = lib.deprecate { year = 2018; month = 8; warnFor = 2; @@ -271,8 +271,9 @@ A sane default is `d = 0, w = 1`, to deprecate immediately and warn for one rele - A package being unable to be built anymore because it would require big changes might be deprecated immediately with no warning phase (`d = 0, w = 0`), such that the value doesn't have to be kept around anymore. - A change that will influence a lot of code and might be hard to migrate from could have an instant deprecation but a warning phase of 4 releases (`d = 0, w = 4`), such that people have a lot of time to migrate away from it. - Aliases might be deprecated only after two years so people enabling all warnings can be alarmed of the eventual removal. The warning phase could then be about 4 releases long because aliases don't cost much anyways (`d = 2 years, w = 4`). +- A package that has a known end-of-support date such as Java 7 in July 2022 can have this reflected in the deprecation time. -This isn't main focus of this RFC, however we will address some issues regarding this. +This isn't main focus of this RFC, however we will address a controversial issues regarding this: ### Aliases @@ -282,12 +283,12 @@ Advantages of removing aliases: 1. Nix evaluation will be faster and use less memory for everybody. Doing a quick analysis using `export NIX_SHOW_STATS=1` on nixpkgs de825a4eaacd and Nix 2.0.4 with either all or (almost) no aliases with [this diff](https://gist.github.com/7f647d15b27825c3cc4dc2079acddb39), these are the results for different commands executed in the nixpkgs root. Time has been measured with an initial unmeasured run and an average over multiple runs after that, standard derivation in parens, in seconds. Heap is deterministic. - | Command | time elapsed (aliases -> no aliases) | total Boehm heap allocations (aliases -> no aliases) | - | --- | --- | --- | - | `nix-instantiate --eval -A path` | (15 runs), 0.043 (0.0041) -> 0.042 (0.0042) | 4.12MB -> 3.89MB | - | `nix-instantiate -A openssl` | (15 runs) 0.095 (0.0060) -> 0.090 (0.0049) | 18.5MB -> 17.7MB | - | `nix-instantiate nixos/release.nix -A iso_graphical` | (15 runs) 5.71 (0.19) -> 5.64 (0.10) | 839MB -> 836MB | - | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | + | Command | time elapsed (aliases -> no aliases) | total Boehm heap allocations (aliases -> no aliases) | + | --- | --- | --- | + | `nix-instantiate --eval -A path` | (15 runs) 0.043 (0.0041) -> 0.042 (0.0042) | 4.12MB -> 3.89MB | + | `nix-instantiate -A openssl` | (15 runs) 0.095 (0.0060) -> 0.090 (0.0049) | 18.5MB -> 17.7MB | + | `nix-instantiate nixos/release.nix -A iso_graphical` | (15 runs) 5.71 (0.19) -> 5.64 (0.10) | 839MB -> 836MB | + | `nix-instantiate nixos/release-combined.nix -A tested` | (5 runs) 222.6 (6.1) -> 218.2 (2.3) | 36.51GB -> 36.35GB | Putting some statistical meaning into the timings using an alpha of 2.5% (z = 1.96) leads to a result of everything but the first command being statistically significant (maybe because it's the only --eval?). So we really got ourselves about 1-3% better speed, which is not much, but it's not nothing. Also 0.4-5% less memory consumption, depending on the expression. 2. Cruft can be removed, cleaner repository state. Not that significant, since aliases are in their own file in `` and are therefore very isolated from everything else. @@ -296,23 +297,9 @@ Disadvantages of removing aliases: 1. Deprecation needed -> People will eventually see warnings, potentially lots of them Doing a quick analysis of alias introduction times using `cat pkgs/top-level/aliases.nix | rg '[0-9]+-[0-9]+-[0-9]+' -o | cut -d- -f-2 | sort | uniq -c`, we get an mean of 5.5 and a median of 3 new aliases per month over the last ~4 years. This means if aliases were to be deprecated at a constant rate, we'd see about 5 new deprecations every month, so about 30 every release. Now of course not everybody uses every nixpkgs attribute, so it will be less than that for individual users. Considering that aliases will only get introduced for popular packages (since unpopular ones very little people use to begin with, so aliases wouldn't get introduced a lot), I estimate that an average user will encounter about 0-2 new alias deprecations per release. This amount will of course be bigger with "power" users, using lots of different packages. Considering that there are quite a few people only upgrading their version every 1-2 years, this can add up. -Assigning weights to +Assigning subjective weights to these points: The first advantage receives a weight of 4, the second advantage a weight of 1, the disadvantage gets a weight of 20 because it's user-facing and annoying. If I didn't forget anything, this adds to a resulting 14 points for *not* deprecating aliases. In short, the cost of deprecating aliases is much bigger than the cost of leaving them in. - -### Cases - -- Unsupported/deprecated (version of a) package, like `foobar_0_1` which is for some reason still in the code -- A package gets renamed for consistency reasons - -## Deprecation messages - -Should include -- Date of deprecation -- (optional) Date of expected removal -- Reason -- (optional) Remedy - -## PRs +## Links https://github.com/NixOS/nixpkgs/pull/32776#pullrequestreview-84012820 @@ -328,19 +315,17 @@ https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-418424080 ## Todo -- Option to silence warnings, option to show warnings -- Introduce automatic capture of attribute path in nixpkgs for better error messages ## Examples Can be used for deprecating versions when they're not supported anymore by upstream at the exact day. -Single file with all deprecations as an overlay? Can't implement everything, what about deprecated function args? What about stuff you can't overlay? - # Drawbacks [drawbacks]: #drawbacks -Why should we *not* do this? +Drawbacks of this approach in comparison to keep doing what has been done until now: +- Deprecating lots of attributes will increase evaluation time, as the conditions for warnings/throws need to be evaluated. +- The usage of the `deprecation` function has to be learned, it's not as simple as just removing the attribute, adding a trace or replacing it with a throw. Adding good docs will help for this. # Alternatives [alternatives]: #alternatives @@ -350,13 +335,13 @@ What other designs have been considered? What is the impact of not doing this? # Unresolved questions [unresolved]: #unresolved-questions -What about errors in library functions? Correct or keep backwards compatible. #41604 -What about function arguments? +- What about errors in library functions? Correct or keep backwards compatible? #41604 +- What about function arguments? +- Introduce automatic capture of attribute path in nixpkgs for better error messages? Is this even possible? # Future work [future]: #future-work -What future work, if any, would be implied or impacted by this feature -without being directly part of the work? +From the scope defined above it should be possible to implement a program that creates an index over nixpkgs, containing every attribute path that's intended to be used (I have a prototype of this working). This can then be used for automatically verifying backwards compatibility for commits. This can also can be used for providing a tags file to look up where functions and attributes are located, which is a much needed feature for nixpkgs. Another possible application of this is to provide documentation for attributes not directly where they're defined, but in a `attribute-docs.nix` file. -From the scope defined above it should be possible to implement a program that creates an index over nixpkgs, containing everything that's intended to be used. This can then be used for automatically verifying backwards compatibility for commits. Also this can be used for providing a tags file to look up functions and attributes. Possibly also for providing an out-of-tree documentation and examples. +Similarly, the above defined allowed state transitions can be used to check that commits or pull requests don't do invalid transitions. Also a periodic automated job could run to set deprecation values to `null` if the values aren't needed anymore. Related, one could implement a program that returns which attributes are in which deprecation stage, doing some statistics with them. From 06c455ca0b3d5314e1a6458908d8a95fdbe7d3c2 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 06:04:37 +0200 Subject: [PATCH 17/24] Add alternatives --- rfcs/0000-deprecation.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index d425a3059..e58ea34b7 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -330,7 +330,9 @@ Drawbacks of this approach in comparison to keep doing what has been done until # Alternatives [alternatives]: #alternatives -What other designs have been considered? What is the impact of not doing this? +- Not doing anything. keep doing deprecation on a case-by-case basis, judging the most appropriate action. +- Having deprecation be a 0/1 thing: Either it's not deprecated and doesn't throw an error, or it is deprecated and it throws an error. The obvious disadvantage of this is that when people upgrade their nixpkgs, they can't expect their code to still work, even if they had no single warning message in the last version. +- Not including the expected removal date in the warning message. The disadvantage of this is that it's just not as nice to the user. Seeing a time of removal gives the user a good sense of either "I need to fix this now" or "This can wait". The advantage of this would be a more flexible deprecation policy, enabling us to remove values at whatever time one wishes. # Unresolved questions [unresolved]: #unresolved-questions From 52d15f66c77a9f65d5f89e9d2b4ba784bea3c371 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 06:06:02 +0200 Subject: [PATCH 18/24] minor cleanup --- rfcs/0000-deprecation.md | 7 ------- 1 file changed, 7 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index e58ea34b7..b49704eb6 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -313,13 +313,6 @@ https://github.com/NixOS/nixpkgs/pull/19315 https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-418424080 -## Todo - - -## Examples - -Can be used for deprecating versions when they're not supported anymore by upstream at the exact day. - # Drawbacks [drawbacks]: #drawbacks From 1e2a30feb78b0f5cf1ef4d58b14ac63151d59796 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Wed, 5 Sep 2018 11:39:46 +0200 Subject: [PATCH 19/24] Move and correct implementation to branch --- rfcs/0000-deprecation.md | 47 ++-------------------------------------- 1 file changed, 2 insertions(+), 45 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index b49704eb6..0a0b2e828 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -122,6 +122,8 @@ The implementation of such a deprecation function will purely based on the plann To make implementation more efficient and easier, a pair of release version year/month will be converted into an integer corresponding to the number of months since year 2000. This means the release version `"18.09"` will be handled as `18 * 12 + 9 = 225`. In the implementation variables that contain such a value will be named `...YearMonth`. +The following has been implemented in my [deprecation branch](https://github.com/infinisil/nixpkgs/tree/deprecation). + ### Release tracking To implement deprecation functions that can vary their behavior depending on the number of releases that have passed since initial deprecation, we need to track past releases. A file `` should be introduced with contents of the form @@ -194,51 +196,6 @@ Which will correspond to } ``` -The basic implementation and meaning of arguments is a follows (Note: I didn't test this yet). -```nix -{ - releaseYearMonth = let - year = lib.toInt (lib.versions.major lib.trivial.release); - month = lib.toInt (lib.versions.minor lib.trivial.release); - in year * 12 + month; - - allReleases = [ releaseYearMonth ] ++ map (r: r.yearMonth) releases; - - takeWhile = pred: list: ...; - - deprecate = - { year # Year of deprecation as an integer - , month # Month of deprecation as an integer - , warnFor ? 1 # Number of releases to warn before throwing - , reason # Reason of deprecation - , value ? null # The value to evaluate to, may be null if the current release throws - }@args: - if warnFor < 0 then abort "the warnFor argument to lib.deprecate needs to be non-negative" else - let - deprecationYearMonth = (year - 2000) * 12 + month; - depReleases = takeWhile (ym: ym > deprecationYearMonth) allReleases; - location = let pos = unsafeGetAttrPos "reason" args; in - pos.file + ":" + toString pos.line; - - isDeprecated = depReleases != []; - deprecationRelease = if isDeprecated then last depReleases else - releaseYearMonth + ((deprecationYearMonth - releaseYearMonth) / 6) * 6); - isRemoved = depReleases > warnFor; - removedRelease = if isRemoved then elemAt depReleases (length depReleases - warnFor) else - releaseYearMonth + ((deprecationYearMonth - releaseYearMonth) / 6 + warnFor) * 6); - prettyRelease = yearMonth: "${toString (div yearMonth 12)}.${toString (mod yearMonth 12)}"; - deprecationString = if isDeprecated then "Deprecated since ${prettyRelease deprecationRelease}" else - "Will be deprecated in ${prettyRelease deprecationRelease}"; - removedString = if isRemoved then ", removed since ${prettyRelease removedRelease}" else - ", will be removed in ${prettyRelease removedRelease}"; - message = "${deprecationString}${removedString} at ${location}. ${reason}"; - in - # TODO: Improve message for warnFor = 0 - if ! isDeprecated then assert value != null; value # TODO: Colors and stuff? - else if isRemoved then throw message - else assert value != null; builtins.trace message value; -} -``` ### Configuration From d09793a90d3765f45d2240900c6e9ece4d21e705 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 8 Sep 2018 01:35:46 +0200 Subject: [PATCH 20/24] rewrite summary, add extra, clean up --- rfcs/0000-deprecation.md | 111 ++++++++++++++++++++++++--------------- 1 file changed, 68 insertions(+), 43 deletions(-) diff --git a/rfcs/0000-deprecation.md b/rfcs/0000-deprecation.md index 0a0b2e828..7114f649f 100644 --- a/rfcs/0000-deprecation.md +++ b/rfcs/0000-deprecation.md @@ -9,26 +9,40 @@ related-issues: (will contain links to implementation PRs) # Summary [summary]: #summary -We propose to add a deprecation guideline and a set of accompanying functions and tools to deprecate functionality in nixpkgs. Instead of removing an attribute which would result in an `attribute missing` error for users, one can deprecate a function, which will then first throw a warning and later throw an error. This is intended to be used for anything accessible from `import {}`, including the standard library, packages, functions, aliases, package sets, etc. +This RFC proposes to add 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 missing` error 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: + +``` +{ + foo = lib.deprecate { + year = 2018; + month = 8; + attrPath = [ "foo" ]; + reason = "Was replaced with bar"; + value = "foo"; + }; +} +``` + +Evaluating `foo` will 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: +``` +trace: WARNING: pkgs.foo is deprecated since 18.09 and will be removed in ~19.03: Was replaced with bar +"foo" +``` # Motivation [motivation]: #motivation -Currently nixpkgs doesn't have any standard way to deprecate functionality. When something is too bothersome to keep around, the attribute often just gets dropped (490ca6aa8ae89d0639e1e148774c3cd426fc699a, 28b6f74c3f4402156c6f3730d2ddad8cffcc3445), leaving users with an `attribute missing` error, which is neither helpful nor necessary, because Nix has functionality for warning the user of deprecation or throwing useful error messages. This approach has been used before, but on a case-by-case basis (6a458c169b86725b6d0b21918bee4526a9289c2f). (Todo: link to 1aaf2be2a022f7cc2dcced364b8725da544f6ff7 and find more commits that remove aliases/packages/function args). +Currently nixpkgs doesn't have any standard way to deprecate functionality. When something is too bothersome to keep around, the attribute often just gets dropped, leaving users with an `attribute missing` error, which is neither helpful nor necessary, because Nix has functionality for warning the user of deprecation or throwing useful error messages. This approach has been used before, but only on a case-by-case basis. -Why are we doing this? What use cases does it support? What is the expected -outcome? +The goal with this RFC is to provide a useful function that can be dropped in whenever deprecation is needed, without having to worry about the specifics. People will get proper error messages, warning them of the upcoming deprecation, and when the planned time has come, throw an error instead of keep warning. This has the effect of motivating people to update their code. # Detailed design [design]: #detailed-design -This is the bulk of the RFC. Explain the design in enough detail for somebody -familiar with the ecosystem to understand, and implement. This should get -into specifics and corner-cases, and include examples of how the feature is -used. - ## Discussion +This section is mainly theory, it's not necessary to understand the RFC, but useful for the ideas described in [future work](#future-work). Feel free to skip to the [implementation section](#implementation). + ### Scope Backwards compatibility should only concern the intended usage of nixpkgs: @@ -117,23 +131,17 @@ This leads us to the following allowed state transitions between our primitives: - `throw d n` -> removed, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. ## Implementation +[implementation]: #implementation The implementation of such a deprecation function will purely based on the planned release version of the next release (e.g. 18.09) and release versions of past releases. An argument could then be made that deprecation dates should also just be a release version, however it appears to be more convenient and less error prone if we allow all year/month combinations. Deprecations can then be added simply by inserting the current or planned deprecation year and month, instead of having to figure out what the next (or later than next) release version is going to be. This will work correctly as long as the `` file always contains a version number that's not in the past. This can indeed occur if the date of effective release stretches itself past the month of the release version, which isn't a big problem however, even if unnoticed (the deprecation will just be delayed by a release). -To make implementation more efficient and easier, a pair of release version year/month will be converted into an integer corresponding to the number of months since year 2000. This means the release version `"18.09"` will be handled as `18 * 12 + 9 = 225`. In the implementation variables that contain such a value will be named `...YearMonth`. - The following has been implemented in my [deprecation branch](https://github.com/infinisil/nixpkgs/tree/deprecation). ### Release tracking To implement deprecation functions that can vary their behavior depending on the number of releases that have passed since initial deprecation, we need to track past releases. A file `` should be introduced with contents of the form ```nix -map ({ release, ... }@attrs: attrs // { - yearMonth = let - year = lib.toInt (lib.versions.major release); - month = lib.toInt (lib.versions.minor release); - in year * 12 + month; -}) [ +[ { name = "Impala"; release = "18.03"; @@ -146,7 +154,7 @@ map ({ release, ... }@attrs: attrs // { release = "17.09"; year = 2017; month = 9; - day = ??; + # day = ??; } { name = "Gorilla"; @@ -159,6 +167,8 @@ map ({ release, ... }@attrs: attrs // { ] ``` +TODO: Maybe omit the year/month/day, because we don't need them for anything, yet at least. + Additional fields can be added if the need arises. This file should be updated at date of release. It may also be updated already at branch-off time (TODO: Think about this some more). ### Deprecation function @@ -170,33 +180,13 @@ A function `lib.deprecate` will be introduced. When an attribute should be depre year = 2018; month = 8; warnFor = 2; + attrPath = [ "foo" ]; reason = "foo has been deprecated, use bar instead"; value = "foo"; }; } ``` -Optionally, for convenience, a shorthand `lib.deprecate'` could be provided as well: -```nix -{ - bar = lib.deprecate' 2018 8 "bar is a burden to the codebase, refrain from using it" "bar"; -} -``` - -Which will correspond to -```nix -{ - bar = lib.deprecate { - year = 2018; - month = 8; - warnFor = 1; - reason = "bar is burden to the codebase, refrain from using it"; - value = "bar"; - }; -} -``` - - ### Configuration Sometimes one might wish to either silence certain warnings or turn on warnings for planned deprecations. To do this, the nixpkgs `config` attribute will be used: @@ -212,9 +202,35 @@ import { } ``` -TODO: Could there warning overrides be used for more than just deprecations? +### Extra + +I have implemented a very interesting and useful but invasive change to nixpkgs in [this branch](https://github.com/infinisil/nixpkgs/tree/deprecation). The change can be separated into 2 parts, which flow hand in hand. TODO: Evaluate nixpkgs performance on this change. -Implementation details: Add a `config` attribute to `lib` with the value `{}` and use it for the deprecation function. This value can be changed by using `lib.extends (self: super: { config = ; } }`. Define `pkgs.lib` as `lib.extends (self: super: { config = config.deprecation; }`. +#### attrPath argument + +The first part calls every function of nixpkgs that has takes a single attribute set argument with a `attrPath` key with the corresponding attribute path. For example, if you have defined `foo` like this in ``: +```nix +{ + foo = { attrPath }: + "You can find me at ${lib.concatStringsSep "." attrPath}"; +} +``` + +Evaluating `foo` will result in `"You can find me at foo"`. + +This change is the one relevant to this PR, because **it allows the deprecation function to automatically know the attribute path of the package**. The user therefore won't have to provide it. This can also be used in the `gnome3` package structure: These attribute paths are needed for creating update scripts, currently they are provided manually for every package. + +#### attrPath and attrPos + +The second part of it extends every attrset in nixpkgs with 2 attributes: `attrPath` and `attrPos`. For example: +``` +$ nix-instantiate --eval -A haskellPackages.xmonad.attrPath +[ "haskellPackages" "xmonad" ] +$ nix-instantiate --eval -A haskellPackages.xmonad.attrPos +{ column = 3; file = "/home/infinisil/src/nixpkgs/pkgs/development/haskell-modules/configuration-nix.nix"; line = 225; } +``` + +`attrPos` is very useful if you want to look up where some attribute is defined without knowing the attribute path to it. TODO: Usecase for `attrPath` and `attrPos`. ## Deprecation process @@ -230,7 +246,7 @@ A sane default is `d = 0, w = 1`, to deprecate immediately and warn for one rele - Aliases might be deprecated only after two years so people enabling all warnings can be alarmed of the eventual removal. The warning phase could then be about 4 releases long because aliases don't cost much anyways (`d = 2 years, w = 4`). - A package that has a known end-of-support date such as Java 7 in July 2022 can have this reflected in the deprecation time. -This isn't main focus of this RFC, however we will address a controversial issues regarding this: +This isn't main focus of this RFC, however I will address a controversial issues regarding this: ### Aliases @@ -254,7 +270,7 @@ Disadvantages of removing aliases: 1. Deprecation needed -> People will eventually see warnings, potentially lots of them Doing a quick analysis of alias introduction times using `cat pkgs/top-level/aliases.nix | rg '[0-9]+-[0-9]+-[0-9]+' -o | cut -d- -f-2 | sort | uniq -c`, we get an mean of 5.5 and a median of 3 new aliases per month over the last ~4 years. This means if aliases were to be deprecated at a constant rate, we'd see about 5 new deprecations every month, so about 30 every release. Now of course not everybody uses every nixpkgs attribute, so it will be less than that for individual users. Considering that aliases will only get introduced for popular packages (since unpopular ones very little people use to begin with, so aliases wouldn't get introduced a lot), I estimate that an average user will encounter about 0-2 new alias deprecations per release. This amount will of course be bigger with "power" users, using lots of different packages. Considering that there are quite a few people only upgrading their version every 1-2 years, this can add up. -Assigning subjective weights to these points: The first advantage receives a weight of 4, the second advantage a weight of 1, the disadvantage gets a weight of 20 because it's user-facing and annoying. If I didn't forget anything, this adds to a resulting 14 points for *not* deprecating aliases. In short, the cost of deprecating aliases is much bigger than the cost of leaving them in. +Assigning *subjective* weights to these points: The first advantage receives a weight of 4, the second advantage a weight of 1, the disadvantage gets a weight of 20 because it's user-facing and annoying. If I didn't forget anything, this adds to a resulting 14 points for *not* deprecating aliases. In short, the cost of deprecating aliases is much bigger than the cost of leaving them in. ## Links @@ -270,6 +286,15 @@ https://github.com/NixOS/nixpkgs/pull/19315 https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-418424080 +https://github.com/NixOS/nixpkgs/pull/45717#issuecomment-419393594 + +https://github.com/NixOS/nixpkgs/commit/490ca6aa8ae89d0639e1e148774c3cd426fc699a +https://github.com/NixOS/nixpkgs/commit/28b6f74c3f4402156c6f3730d2ddad8cffcc3445 +https://github.com/NixOS/nixpkgs/commit/6a458c169b86725b6d0b21918bee4526a9289c2f +https://github.com/NixOS/nixpkgs/commit/1aaf2be2a022f7cc2dcced364b8725da544f6ff7 + +TODO: link to and find more commits that remove aliases/packages/function args + # Drawbacks [drawbacks]: #drawbacks From 5ce4b2fbd81a267e16cbd83ce64c80ae7ab4d8ad Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 8 Sep 2018 01:54:50 +0200 Subject: [PATCH 21/24] Move to 0033 --- rfcs/{0000-deprecation.md => 0033-deprecation.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rfcs/{0000-deprecation.md => 0033-deprecation.md} (100%) diff --git a/rfcs/0000-deprecation.md b/rfcs/0033-deprecation.md similarity index 100% rename from rfcs/0000-deprecation.md rename to rfcs/0033-deprecation.md From 93db63f3122003f403b9e6528211f679770e3058 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 8 Sep 2018 22:16:08 +0200 Subject: [PATCH 22/24] Add WIP function argument deprecations section --- rfcs/0033-deprecation.md | 71 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/rfcs/0033-deprecation.md b/rfcs/0033-deprecation.md index 7114f649f..0901cba9f 100644 --- a/rfcs/0033-deprecation.md +++ b/rfcs/0033-deprecation.md @@ -130,6 +130,77 @@ This leads us to the following allowed state transitions between our primitives: - `warn d n val` -> `removed`, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. - `throw d n` -> removed, if `r >= d + n + long`, aka it should be a while before we can remove throw messages. `long` stands for the number of releases the `throw` should have been issued for. +### Function argument deprecations + +Possible incompatibilities: +- Arg changed from optional (with default) -> mandatory +- Arg changed supported input values + - old value can be converted to new value + - old value can't be converted to new value +- Arg removed, that functionality not supported anymore +- Arg replaced + - old arg can be converted to new arg + - old arg can't be converted to new arg +- Arg list was previously vararg, but now isn't anymore + +#### Optional (with default) -> mandatory + +Nix: { foo ? "foo" } -> { foo } +Nix: { ... } -> { foo, ... }: +Detectable with functionArgs: Yes +Detectable with args: Yes +What do: + - If a suitable default value for the argument can still be provided: prewarn, warn, throw + - Otherwise: throw + +#### Valid inputs changed (such that not all permitted old values work with the new argument) + +Nix: { foo } -> { foo } +Detectable with functionArgs: No +Detectable with args: Yes +What do: + - If the invalid value can be converted to a new valid value: prewarn, warn, throw + - Otherwise: throw + +#### Argument removed + +Nix: { foo } -> { foo ? null }@args: if args ? foo then warn -> { foo ? null }@args: if args ? foo then throw -> { } +Nix: { foo ? "foo" } -> { } +Detectable with functionArgs: Yes +Detectable with args: Yes +What do: + - If the old argument didn't have any effect anyways: prewarn, warn, throw + - Otherwise: throw + +#### Argument removed (with vararg) + +Nix: { foo } -> { ... } +Nix: { foo ? "foo", ... } -> { ... } +Detectable with functionArgs: Yes (but indistinguishable with previous one) +Detectable with args: No +What do: Can't do anything, only change this if the behaviour doesn't change + +#### Argument renamed/changed + +Nix: { foo } -> { foo ? null, bar }@args: if args ? foo then warn -> { foo ? null, bar }@args: if args ? foo then throw -> { bar } +Nix: { foo ? "foo" } -> { bar } +Detectable with functionArgs: Yes +Detectable with args: Yes +What do: + - If the old argument value can be converted into the new one: prewarn, warn, throw + - Otherwise: throw + +#### vararg -> not vararg + +Nix: { ... } -> { foo, ... }@args: (doesn't work when input only contained foo already) if attrNames args != [ "foo" ] then warn -> { foo } +Nix: { foo, ... } -> { foo, ... }@args: if attrNames args != [ "foo" ] then warn -> { foo } +Detectable with functionArgs: No +Detectable with args: Yes +What do: + - If old arguments can be converted into new one: prewarn, warn, throw + - Otherwise: throw + + ## Implementation [implementation]: #implementation From d685c52ea6beb8a9d66a9c6fcd8b873fcad89adf Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Sat, 8 Sep 2018 22:20:44 +0200 Subject: [PATCH 23/24] Adding section on removing deprecated usage from nixpkgs --- rfcs/0033-deprecation.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rfcs/0033-deprecation.md b/rfcs/0033-deprecation.md index 0901cba9f..b70e87c45 100644 --- a/rfcs/0033-deprecation.md +++ b/rfcs/0033-deprecation.md @@ -317,7 +317,9 @@ A sane default is `d = 0, w = 1`, to deprecate immediately and warn for one rele - Aliases might be deprecated only after two years so people enabling all warnings can be alarmed of the eventual removal. The warning phase could then be about 4 releases long because aliases don't cost much anyways (`d = 2 years, w = 4`). - A package that has a known end-of-support date such as Java 7 in July 2022 can have this reflected in the deprecation time. -This isn't main focus of this RFC, however I will address a controversial issues regarding this: +### Ridding nixpkgs of deprecated usage + +When something is being deprecated, one has to ensure to rid nixpkgs of all usages of the deprecated value. Otherwise the user could get warnings or error message that didn't stem from their code. ### Aliases From f15601c6705e769243f34438307f631243408ab7 Mon Sep 17 00:00:00 2001 From: Silvan Mosberger Date: Mon, 10 Sep 2018 14:35:22 +0200 Subject: [PATCH 24/24] Set co-author --- rfcs/0033-deprecation.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/0033-deprecation.md b/rfcs/0033-deprecation.md index b70e87c45..7fc02405b 100644 --- a/rfcs/0033-deprecation.md +++ b/rfcs/0033-deprecation.md @@ -2,7 +2,7 @@ feature: deprecation start-date: 2018-08-25 author: Silvan Mosberger (@infinisil) -co-authors: (find a buddy later to help our with the RFC) +co-authors: John Ericson (@Ericson2314) related-issues: (will contain links to implementation PRs) ---