Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: deprecation, warnRenamed, release constants #19315

Closed
wants to merge 1 commit into from

Conversation

Profpatsch
Copy link
Member

Introduce the concept of attribute deprecation.
A new library contains deprecation functions (for now only
renamedWarning) that annotate functions and print a deprecation
warning.

In order to manage time until a symbol is removed, we introduce release
constants that are used in the warning messages and contain release
names and dates.

The printed warning only uses a trace message at the moment, this should
be improved.

As an example, opusTools is renamed to opus-tools, with the
reasoning that the related vorbis-tools uses a different naming
scheme. Users on master will immediately get a warning, users of stable
channels can find out about the changed attribute from the changelog
when they update to a new release.

cc @edolstra @aszlig @vcunat

@mention-bot
Copy link

@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @nbp and @7c6f434c to be potential reviewers.

@globin
Copy link
Member

globin commented Oct 7, 2016

(bikeshedding) I'd prefer changing renamedWarning to warnRenamed.

Additionally using lib.warn adds a "WARNING: " prefix if you use it instead of builtins.trace.

@Profpatsch
Copy link
Member Author

Profpatsch commented Oct 7, 2016

(bikeshedding) I'd prefer changing renamedWarning to warnRenamed.

Thanks, I was dissatisfied with the name myself.
See you in an hour? :)

@Profpatsch Profpatsch changed the title lib: deprecation, renameWarning, release constants lib: deprecation, warnRenamed, release constants Oct 7, 2016
Introduce the concept of attribute deprecation.
A new library contains deprecation functions (for now only
`renamedWarning`) that annotate functions and print a deprecation
warning.

In order to manage time until a symbol is removed, we introduce release
constants that are used in the warning messages and contain release
names and dates.

As an example, `opusTools` is renamed to `opus-tools`, with the
reasoning that the related `vorbis-tools` uses a different naming
scheme. Users on master will immediately get a warning, users of stable
channels can find out about the changed attribute from the changelog
when they update to a new release.
@Profpatsch
Copy link
Member Author

I remember seeing a checklist for releases somewhere. We should add “search and delete all that is deprecated in this version, add each item to the release notes; do it in one commit”.

@edolstra
Copy link
Member

edolstra commented Oct 7, 2016

This has been tried before, but the problem is that it makes nix-env -qa print out lots of warnings (because it evaluates deprecated attributes).

@domenkozar
Copy link
Member

@Profpatsch just landed in master at ed6ea74

@Profpatsch
Copy link
Member Author

I’m not sure, I haven’t used nix-env for over a year I think. I hear it does … strange things. :)
Can we add some metadata it can use to find out about deprecations? I’d like to get rid of the special “skipping abort and error from nix-env sooner or later, it makes a lot of things very awkward.

@sternenseemann
Copy link
Member

sternenseemann commented Oct 7, 2016

Still to do would be

  • warnRemoved
  • warnSemanticChange (new package behind the name etc.)

, releaseDate
}: "${name} (release date ${releaseDate})";

next = nixos-17-first;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be 17.09 (not 17.03)? If it's 17.03, there is no deprecation period for users of stable channels.

Copy link
Member Author

Choose a reason for hiding this comment

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

not 17.03)? If it's 17.03, there is no deprecation period for users of stable channels.

When you update your stable channel you are expected to read the changelog and fix any evaluation errors that arise in one swoop. At least for renames it gives a nice evaluation error and will be documented in the changelog.

@vcunat
Copy link
Member

vcunat commented Oct 16, 2016

Ref. about problems with nix-env: 8e8e23de33

@Profpatsch
Copy link
Member Author

At the moment we only have trace as a way of putting stuff on stderr while evaluating. trace was originally designed to enable debugging, but has been abused a lot for warnings.

What do you think about introducing a warning <msg> builtin, which prints a yellow Warning: to stderr? Those can then be ignored by tools that want to strictly evaluate as much of nixpkgs as possible, and we get trace back for debugging, as was originally intended.
cc @edolstra @vcunat.

@vcunat
Copy link
Member

vcunat commented Nov 7, 2016

Yeah, perhaps. Tools can filter those lines easily enough now (e.g. by a simple grep), though it's not perfectly reliable in principle.

@Profpatsch
Copy link
Member Author

@vcunat With your changes to mkDerivation recently, do you think the things in this PR could work now?

@samueldr
Copy link
Member

It's probably a bit late to get this together for 18.09, but I really think either this or something doing the same job is in need.

There were a couple sparks flying around lately with regards to deprecation of attributes;

We had a chat about something similar recently on #nixos. One main thing differed from your approach in the idea thrown around: instead of approaching the deprecation in two stages (deprecate, remove), add an additional step (pre-deprecate, deprecate, remove). The pre-deprecation step would allow through an opt-in configuration to get the warnings during the pre-deprecation period.

The three-step approach allows users not to worry with a deprecation warning stemming from some tooling they use. Imagine tool "A" using nix, and using the attribute foo which has just been deprecated for the next NixOS release cycle. The author of "A" may not be able to fix their tool in time with the release, and the now-deprecated option shows up in their user systems, somehow confusing them. Not the most amount of value for use inside nixpkgs, but probably much more for third-parties building solutions around nix and nixpkgs. (I know my users at $client would all let me know about a new deprecation warning!)

Let me know your thoughts about that additional step.


TLDR: 👍 want to see this or something doing the same job hit 19.03.

@Profpatsch
Copy link
Member Author

Profpatsch commented Aug 24, 2018

instead of approaching the deprecation in two stages (deprecate, remove), add an additional step (pre-deprecate, deprecate, remove)

I wouldn’t overcomplicate it, someone has to remember to change these after all. I haven’t seen systems that do it like this so far. Maybe the overhead is not worth the gains in practice..

@edolstra
Copy link
Member

What really is the need for removing compability aliases? It doesn't really cost us anything to keep an alias like opusTools = opus-tools around.

Also, removedAt = lib.releases.next is inherently stateful - if people actually write that, it would mean the package would never be removed since its removal is always in the future. (Or you would have to consult the Git history to see when the removedAt line was first added.)

@samueldr
Copy link
Member

I am operating only under the assumption that there is a need or want to remove compatibility aliases.

Though, something similar would be needed if something is going to be dropped, right? Dropping something from Nixpkgs will cause issues to the end-users (we can easily manage the change inside Nixpkgs).


About complicating with pre-deprecation:

This is for end-user user-friendliness. The warnings will cause headaches to the end-user using additional tools built on top of nixpkgs. The integrator (a nixpkgs user that built the tool built on top of nixpkgs) would be expected to follow up with the news (reading the release notes, hoping it made it in) or better with the option, using the the option opting-in for the warning one release beforehand. This is not for the simple "nixpkgs and nixpkgs users" situation, but one that has to be thought of, users using tools built by other users of nixpkgs.


I will add a bit, and maybe be a bit harsh:

Let's not stop or stall the discussion about the deprecation feature because there is no need since we can keep compatibility aliases. Please either review under the assumption the feature being implemented in whole is required and needed, or reject the feature as a whole if undesirable. Right now the half-rejection-because-we-keep-everything makes this a less desirable feature to look at and work on since there's a heavyweight decision maker casting a negative cloud over the future of the feature.

In the event the feature is desirable only or some uses, and a technical solution does not exist restricting its use (e.g. dropping aliases for renamed attributes), it'll have to be documented and rely on the committers/members to review appropriately.

@mmahut
Copy link
Member

mmahut commented Aug 7, 2019

Any updates on this pull request, please?

@Profpatsch
Copy link
Member Author

Don’t think this will ever happen. Thanks for triaging.

@Profpatsch Profpatsch closed this Aug 9, 2019
@jtojnar jtojnar mentioned this pull request Aug 20, 2019
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants