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.

Sign up for GitHub

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

Jump to bottom

lib: add mdDoc back in to provide proper warning; set explicit timeline for removal #304277

Merged
merged 2 commits into from
Apr 16, 2024
Merged

lib: add mdDoc back in to provide proper warning; set explicit timeline for removal #304277

merged 2 commits into from
Apr 16, 2024

Conversation

eclairevoyant
Copy link
Contributor

@eclairevoyant eclairevoyant commented Apr 15, 2024 •
edited
Loading

Description of changes

Ref. #300735 (comment)

Most uses of mdDoc would be via lib.mdDoc, not lib.options.mdDoc, so we want to keep lib.mdDoc available for consumers to actually receive a useful warning and have enough time to remove usages.

Also, we should provide an explicit timeline for full removal, which I've done in the warning message.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Apr 15, 2024
@eclairevoyant eclairevoyant requested a review from roberth as a code owner April 15, 2024 13:33
@github-actions github-actions bot added the 6.topic: module system About "NixOS" module system internals label Apr 15, 2024
@eclairevoyant eclairevoyant changed the title lib: add mdDoc back in to provide proper warning lib: add mdDoc back in to provide proper warning; set explicit timeline for removal Apr 15, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 15, 2024
philiptaron
philiptaron approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I think this is directionally the right way to deprecate a symbol from lib. There's a whole file of these in lib/deprecated.nix. I wonder how to make this one not just be a one-off.

@eclairevoyant
Copy link
Contributor Author

Does that mean we should move the impl from options.nix to deprecated.nix as well?

@philiptaron
Copy link
Contributor

philiptaron commented Apr 15, 2024 •
edited
Loading

Does that mean we should move the impl from options.nix to deprecated.nix as well?

I think so? After that, pulling that impl into options.nix through inherit (lib.misc) mdDoc or similar. For some reason, the filename is deprecated.nix but the exported name is misc. 🤔

Personally I'm fine to merge as-is, just putting the thought out there about the general "how do things in lib get deprecated?" question.

@eclairevoyant
Copy link
Contributor Author

That's fair.
I checked for out-of-tree usages, and maybe moving to deprecated.nix is not a huge issue, since it seems like almost no one is using lib.options.mdDoc anyway:

https://github.com/search?q=lang%3Anix+%22lib.options.mdDoc%22&type=code
https://github.com/search?q=lang%3Anix+%22%28lib.options%29%22+%22mdDoc%22&type=code

philiptaron
philiptaron requested changes Apr 15, 2024
View reviewed changes
lib/default.nix Show resolved Hide resolved
philiptaron
philiptaron approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment •
edited
Loading

Choose a reason for hiding this comment

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

OK, this PR:

  1. Moves the deprecation warning from lib/options.nix to lib/deprecated.nix.
  2. Re-introduces the symbol mdDoc exported from lib.
  3. Removes the symbol from lib.options.

The final step is the only one that I think might want to be changed, by adding a inherit (lib.misc) mdDoc in lib/options.nix. But I'm fine if it doesn't.

@eclairevoyant
Copy link
Contributor Author

eclairevoyant commented Apr 15, 2024 •
edited
Loading

adding a inherit (lib.misc) mdDoc in lib/options.nix

Done, added a comment there as well.
Also updated PR desc.

@roberth
Copy link
Member

roberth commented Apr 15, 2024

I've done many lib reviews, but I can't remember ever interacting with deprecated.nix. We've been doing deprecations in their normal locations - right in the sublibrary where it always was.
I think we should get rid of deprecated.nix, in a separate PR, and avoid it here.

@eclairevoyant
Copy link
Contributor Author

Okay, thanks for the note; I'll just drop the last commit then.

philiptaron
philiptaron approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

I still wonder about a coherent deprecation policy for lib. As originally noted, I'm fine deprecating in place.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Apr 15, 2024
@roberth roberth merged commit 6af5f24 into NixOS:master Apr 16, 2024
24 checks passed
@roberth roberth mentioned this pull request Apr 16, 2024
Merged
13 tasks
@eclairevoyant eclairevoyant deleted the mddoc-fix branch April 16, 2024 14:44
@wegank wegank mentioned this pull request Apr 16, 2024
Closed
@Jappie3 Jappie3 mentioned this pull request Apr 16, 2024
Merged
@roberth roberth mentioned this pull request Apr 21, 2024
Merged
13 tasks
@roberth
Copy link
Member

roberth commented Apr 21, 2024

I think we should get rid of deprecated.nix, in a separate PR, and avoid it here.

Can't quite remove it anytime soon, but this should help:

@lorenzleutgeb lorenzleutgeb mentioned this pull request Apr 22, 2024
Closed
infinisil pushed a commit that referenced this pull request Apr 22, 2024
@roberth
lib: Document status of deprecated.nix and move it
33e0242 
This is all I could find after co-maintaining lib for a long time.
I've had the fortune of basically not really noticing this file,
because it has had very few interactions until the confusion in
#304277

It seems to be a state of limbo, which would be nice to resolve
(with great care), but this is not urgent, and first we should
document its status.
github-actions bot pushed a commit to nix-community/nixpkgs.lib that referenced this pull request Apr 28, 2024
@roberth
lib: Document status of deprecated.nix and move it
6c7a924 
This is all I could find after co-maintaining lib for a long time.
I've had the fortune of basically not really noticing this file,
because it has had very few interactions until the confusion in
NixOS/nixpkgs#304277

It seems to be a state of limbo, which would be nice to resolve
(with great care), but this is not urgent, and first we should
document its status.
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request May 16, 2024
@roberth
lib: Document status of deprecated.nix and move it
7ef9b9e 
This is all I could find after co-maintaining lib for a long time.
I've had the fortune of basically not really noticing this file,
because it has had very few interactions until the confusion in
NixOS/nixpkgs#304277

It seems to be a state of limbo, which would be nice to resolve
(with great care), but this is not urgent, and first we should
document its status.
@benaryorg
Copy link
Contributor

benaryorg commented Oct 2, 2024 •
edited
Loading

Given that the explicit timeline (lib.mdDoc will be removed from nixpkgs in 24.11) was introduced in this issue I wanted to ask whether there are plans to actually remove it, given that the release preparations for 24.11 are currently ongoing.?

The only consumer of lib.mdDoc that I currently know of (colmena) has code checking for its existence and falls back to an identity function for its implementation removed this recently, so at least in my (comparatively small? I guess?) codebase removal is absolutely feasible though I don't know much about code outside that.
Either way getting that message on 24.11 might cause confusion, so removal or adjusting the warning text would be cool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roberth roberth roberth approved these changes

@philiptaron philiptaron philiptaron approved these changes

@infinisil infinisil Awaiting requested review from infinisil

Assignees
No one assigned
Labels
6.topic: lib The Nixpkgs function library 6.topic: module system About "NixOS" module system internals 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 2 This PR was reviewed and approved by two reputable people
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eclairevoyant @philiptaron @roberth @benaryorg @wegank