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

treewide: remove lib.mdDoc #303841

Merged
merged 4 commits into from
Apr 13, 2024
Merged

treewide: remove lib.mdDoc #303841

merged 4 commits into from
Apr 13, 2024

Conversation

stuebinm
Copy link
Contributor

Motivation

lib.mdDoc has been a no-op (i.e., been defined as mdDoc = id) since the completion of the docs migration and the removal of Docbook about ten months ago. Nevertheless, it is still used in many, many places, and it seems unrealistic to remove it from each module one-by-one (tracking issue on this).

Description of changes

This removes all uses of lib.mdDoc anywhere in nixpkgs. As was discussed in the docs matrix channel, it also replaces mdDoc's definition with a call to lib.warn giving a hint to downstream module authors that they should likewise remove its use.

The string mdDoc remains in nixpkgs in only four cases: its own definition, once as a literal string in the nixos-render-docs python script (where it can probably be removed as well, but I'm not familiar enough with it to make changes there), and twice in commented-out nix code.

The manuals built before and after this change are identical (according to diffoscope the only change is a store path referencing the module directory, where change is unavoidable if we change the nix source files).

Implementation details

As with #297084 I generated these changes via my own rnix-based rust utility, which is syntax-aware enough to both find actual use-sites & hopefully smart enough to remove then-redundant parens and whitespace, leaving the actual nix code in (i think) a reasonably clean-looking state.

I've also split this up into several commits (one each for changing nixos, lib, and pkgs/top-level/config.nix, and a final one for changing mdDoc's definition) in hopes of making it at least slightly less unwieldy to review.

(note for reviewers: re-generating these changes takes only a few seconds, so please don't worry about this branch going out-of-sync with master quickly; if need be, I can regenerate the same changes on a more current master before merging)

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.

these changes were generated with nixq 0.0.2, by running

  nixq ">> lib.mdDoc[remove] Argument[keep]" --batchmode nixos/**.nix
  nixq ">> mdDoc[remove] Argument[keep]" --batchmode nixos/**.nix
  nixq ">> Inherit >> mdDoc[remove]" --batchmode nixos/**.nix

two mentions of the mdDoc function remain in nixos/, both of which
are inside of comments.

Since lib.mdDoc is already defined as just id, this commit is a no-op as
far as Nix (and the built manual) is concerned.
this commit is a no-op, as lib.mdDoc was already defined as id.
this change is otherwise a no-op, as lib.mdDoc is already defined to be
the identity function.
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: qt/kde 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: emacs Text editor 6.topic: printing 6.topic: vim labels Apr 13, 2024
@philiptaron
Copy link
Contributor

@stuebinm this is fantastic, and thanks for nixq also.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-darwin-is-broken/43497/2

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-darwin-is-broken/43497/3

@jjpe
Copy link

jjpe commented Apr 16, 2024

Unfortunately this PR breaks nix-darwin.
It seems there are packages there that have not yet had the requisite maintenance.

@mweinelt
Copy link
Member

If this broke nix-darwin, then they need to remove their usage of lib.mdDoc, that should be all.

@jjpe
Copy link

jjpe commented Apr 16, 2024

True that may be, but I ran into this as someone relatively new to Nix-the-language.
As such, I unfortunately have no experience fixing this kind of issue just yet.
Which means all I can do at this time is draw attention to it, and hope that someone who does have the required knowledge will fix it.

@roberth
Copy link
Member

roberth commented Apr 16, 2024

If this broke nix-darwin, then they need to remove their usage of lib.mdDoc, that should be all.

We try to have a friendly deprecation cycle in lib so that this kind of thing is at most a warning that can be ignored by users and acted on by maintainers.
Unfortunately, this PR was merged hastily, understandably.
Solution in

@jonringer
Copy link
Contributor

jonringer commented Apr 16, 2024

Yes, was most concerned that the scope of this PR would cause it to go into "conflict limbo".

Removal of the lib.mdDoc entry was on me, should be an eval warning, not a hard "mdDoc could not be found" error.

Edit: thank you @eclairevoyant for the fix

@RaitoBezarius
Copy link
Member

For the next urgent merge, note that OP wrote:

(note for reviewers: re-generating these changes takes only a few seconds, so please don't worry about this branch going out-of-sync with master quickly; if need be, I can regenerate the same changes on a more current master before merging)

Using a codemod tool, there's no hurry to regenerate those changes, which makes those treewides possible to discuss without going too fast.

Thank you @eclairevoyant for the fix.

@jonringer
Copy link
Contributor

Maybe having treewide re-writes and deprecations be separate PRs should be the norm in the future. One has a lot of bulk changes but little implications out-of-tree, and the other does not have many changes but many implications out-of-tree.

diniamo added a commit to diniamo/home-manager that referenced this pull request Apr 16, 2024
spotify-player: tests

chore: format

fix: declare in modules.nix

spotify-player: add news entry

spotify-player: implement review suggestions

spotify-player: fix maintainers

Squashed commit of the following:

commit 1c43dcf
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Apr 15 09:42:51 2024 +0200

    ci: bump peaceiris/actions-gh-pages from 3 to 4

    Bumps [peaceiris/actions-gh-pages](https://github.com/peaceiris/actions-gh-pages) from 3 to 4.
    - [Release notes](https://github.com/peaceiris/actions-gh-pages/releases)
    - [Changelog](https://github.com/peaceiris/actions-gh-pages/blob/main/CHANGELOG.md)
    - [Commits](peaceiris/actions-gh-pages@v3...v4)

    ---
    updated-dependencies:
    - dependency-name: peaceiris/actions-gh-pages
      dependency-type: direct:production
      update-type: version-update:semver-major
    ...

    Signed-off-by: dependabot[bot] <[email protected]>
    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit 59d50bc
Author: Nathan Henrie <[email protected]>
Date:   Mon Apr 15 01:40:27 2024 -0600

    espanso: enable module on darwin

commit 9f32c66
Author: Jose Plana <[email protected]>
Date:   Wed Apr 10 13:34:46 2024 +0200

    k9s: configuration files in Darwin without XDG

    Support alternate configuration files for k9s in darwin where XDG is
    not mandated and k9s expects configuration files in
    `~/Library/Application Support/k9s/`.

commit 76a1650
Author: Jose Plana <[email protected]>
Date:   Wed Apr 10 10:24:46 2024 +0200

    k9s: fix typos in configuration file names

commit 630a099
Author: Philipp Mildenberger <[email protected]>
Date:   Sun Apr 14 08:58:16 2024 +0200

    nushell: fix nushell config path on darwin

commit 4cc3c91
Author: home-manager-bot <[email protected]>
Date:   Sun Apr 14 08:56:05 2024 +0200

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)
      → 'github:NixOS/nixpkgs/1042fd8b148a9105f3c0aca3a6177fd1d9360ba5' (2024-04-10)

    Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

commit f33d508
Author: Mitchell Skaggs <[email protected]>
Date:   Sat Apr 13 12:59:33 2024 -0500

    rio: remove redundant `lib.mdDoc` call

    This is an error as of NixOS/nixpkgs#303841

    It seems to have been missed in nix-community#4215

commit 8fdf329
Author: MiSumiSumi <[email protected]>
Date:   Sat Apr 13 23:50:15 2024 +0900

    neovim: enable use of external package manager (nix-community#5225)

    * neovim: add extraWrapperArgs option

    pass external arguments to neovim-unwrapper
    this gives users more flexibility in managing neovim configuration

    * neovim: add test for `extraWrapperArgs`

commit 40ab43a
Author: Bryn Edwards <[email protected]>
Date:   Sat Apr 13 07:27:43 2024 +0100

    foot: set PATH in server's systemd unit file

    If not set, foot's terminal spawning shortcut will not work as the
    `footclient` binary is not on the server's PATH.

commit 3135748
Author: Ramses <[email protected]>
Date:   Wed Apr 10 16:39:52 2024 +0200

    fish: use the subcommand style for the status command (nix-community#4584)

    The flag style has been deprecated and will eventually be removed.

commit 18f89ef
Author: Tony Zorman <[email protected]>
Date:   Wed Apr 10 08:29:32 2024 +0200

    firefox: add containersForce flag

    Firefox, upon exit, creates the default containers.json file in place of
    the one that home-manager created. This leads to errors when switching
    to a new profile, as home-manager is careful with overwriting existing
    files. The added option toggles that behaviour.

    Closes: nix-community#4989

commit b00d0e4
Author: Jack W <[email protected]>
Date:   Tue Apr 9 14:48:15 2024 -0400

    bun: add module

commit 40a9961
Author: Mario Rodas <[email protected]>
Date:   Tue Apr 9 01:57:29 2024 -0500

    fzf: add compatibility with fzf≥0.48.0

    fzf 0.48.0 [1] changed the way it integrates with shells.

    [1] https://github.com/junegunn/fzf/releases/tag/0.48.0

commit a561ad6
Author: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Date:   Sun Apr 7 03:59:32 2024 +0000

    flake.lock: Update

    Flake lock file updates:

    • Updated input 'nixpkgs':
        'github:NixOS/nixpkgs/d8fe5e6c92d0d190646fb9f1056741a229980089' (2024-03-29)
      → 'github:NixOS/nixpkgs/fd281bd6b7d3e32ddfa399853946f782553163b5' (2024-04-03)

commit b787726
Author: Smaug123 <[email protected]>
Date:   Fri Apr 5 14:05:00 2024 +0100

    home-manager: extract inline shell script to file
acid-bong added a commit to acid-bong/nixpkgs-pr that referenced this pull request Apr 17, 2024
- Fixed the `systemPackages` definition: it contained just the package
  name without preceding `pkgs`
- Removed `lib.mdDoc` usage in accordance with NixOS#303841
SuperSandro2000 pushed a commit that referenced this pull request Apr 17, 2024
)

- Fixed the `systemPackages` definition: it contained just the package
  name without preceding `pkgs`
- Removed `lib.mdDoc` usage in accordance with #303841
sandydoo added a commit to cachix/git-hooks.nix that referenced this pull request Jun 20, 2024
Noodlez1232 pushed a commit to Noodlez1232/home-manager that referenced this pull request Nov 21, 2024
This is an error as of NixOS/nixpkgs#303841

It seems to have been missed in nix-community#4215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cinnamon Desktop environment 6.topic: emacs Text editor 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: flakes The experimental Nix feature 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: lib The Nixpkgs function library 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: module system About "NixOS" module system internals 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: pantheon The Pantheon desktop environment 6.topic: printing 6.topic: qt/kde 6.topic: systemd 6.topic: vim 6.topic: xfce The Xfce Desktop Environment 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants