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

nixos/doc/md-to-db.sh: consistent pandoc version #168598

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

alyssais
Copy link
Member

@alyssais alyssais commented Apr 14, 2022

Description of changes

Until now, this script has used the version of pandoc from unstable.
This means that running the script on the same version of Nixpkgs
could produce different results, and meant that when Pandoc's output
was changed, random PRs were changing the whole manual when they ran
the script to regenerate docs. 1 2

Here I've changed the manual to use a consistent version of pandoc —
the one from the latest release tag, which will avoid this problem in
future. This will avoid this problem in future. The only time we'll
need to worry about pandoc output changes is when we bump the version
used in this script.

I also considered using the version from the current Nixpkgs branch,
but decided against it as it's unlikely that e.g. the person bumping
Pandoc will remember to regenerate the manual.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

Until now, this script has used the version of pandoc from unstable.
This means that running the script on the same version of Nixpkgs
could produce different results, and meant that when Pandoc's output
was changed, random PRs were changing the whole manual when they ran
the script to regenerate docs[1][2].

Here I've changed the manual to use a consistent version of pandoc —
the one from the latest release tag, which will avoid this problem in
future.  This will avoid this problem in future.  The only time we'll
need to worry about pandoc output changes is when we bump the version
used in this script.

I also considered using the version from the current Nixpkgs branch,
but decided against it as it's unlikely that e.g. the person bumping
Pandoc will remember to regenerate the manual.

[1]: NixOS#162550
[2]: NixOS#168535
@alyssais alyssais added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Apr 14, 2022
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Apr 14, 2022
@alyssais alyssais mentioned this pull request Apr 14, 2022
13 tasks
@alyssais alyssais requested review from pennae and ckiee April 14, 2022 08:12
@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 14, 2022
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Sounds good, the pandoc in the tag should already contain all of our relevant patches (the patch in b0c6127 is only needed for going in the other direction). And we can always replace the tag with a commit when we want to update it.

Copy link
Contributor

@aforemny aforemny left a comment

Choose a reason for hiding this comment

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

LGTM, let's do it! :-)

@dasJ
Copy link
Member

dasJ commented Apr 14, 2022

I like the idea of this and I can understand why we'd want this but who will remember to update the channel here when 21.11 goes EOL?
Afaic, we are currently not pinning nixpkgs from within nixpkgs and I'd really like this to stay this way because it's very easy to forget one or two uses of the current release.

@rnhmjoj
Copy link
Contributor

rnhmjoj commented Apr 14, 2022

I like the idea of this and I can understand why we'd want this but who will remember to update the channel here when 21.11 goes EOL?

What about using pandoc from the current git checkout? The only problem would be if pandoc is broken for reasons unrelated the current PR, but that should be sorted out by rebasing once the problem is fixed.

Copy link
Contributor

@pennae pennae left a comment

Choose a reason for hiding this comment

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

looks good. maybe pandoc aging as this script isn't touched much any more will add to the motivation to move the docs to markdown :) if it doesn't then at least the manual build isn't particularly sensitive, so not getting pandoc updates for a bit shouldn't actually hurt anyone

@alyssais
Copy link
Member Author

@rnhmjoj

What about using pandoc from the current git checkout? The only problem would be if pandoc is broken for reasons unrelated the current PR, but that should be sorted out by rebasing once the problem is fixed.

From the PR body:

I also considered using the version from the current Nixpkgs branch, but decided against it as it's unlikely that e.g. the person bumping Pandoc will remember to regenerate the manual.

This would not have prevented the problems seen today.

@alyssais
Copy link
Member Author

alyssais commented Apr 14, 2022

@dasJ

I like the idea of this and I can understand why we'd want this but who will remember to update the channel here when 21.11 goes EOL?

EOL is irrelevant, because this is a tag, not a channel. It went EOL as soon as the first commit was made on top of the tag. The point is that it doesn't matter if it's out of date, as long as it generates the correct output. If we're missing something from a new pandoc, we can bump it. Security issues, etc. are irrelevant because the content of the repository (i.e. pandoc's input) is already trusted by any Nixpkgs user.

Afaic, we are currently not pinning nixpkgs from within nixpkgs and I'd really like this to stay this way because it's very easy to forget one or two uses of the current release.

What is your proposed alternative?

@dasJ
Copy link
Member

dasJ commented Apr 14, 2022

I have no real suggestion tbh. Something that would be nice (but I don't know if there would be any other consumers) could be to define something like lib.versions.currentStable (?) and use that for this pin and other pins.

Also, to make my statements more clear: While I don't like this PR 100%, I don't want to block it from being merged. As you said, it's not relevant to have the newest pandoc version for building the manual, I also want to prevent nixpkgs from being pinned and kept at an old release because changes tend to stack up over multiple releases and switching 4 releases or so in one commit is a lot more painful than going with every release and only fixing the small issues that pop up.

@alyssais alyssais merged commit b825f6d into NixOS:master Apr 14, 2022
@alyssais alyssais deleted the md-to-db-reproducible branch April 14, 2022 12:57
@alyssais
Copy link
Member Author

I have no real suggestion tbh. Something that would be nice (but I don't know if there would be any other consumers) could be to define something like lib.versions.currentStable (?) and use that for this pin and other pins.

Would it be possible to use that in a nix-shell shebang? I'd be surprised, but don't feel confident enough to categorically say it wouldn't be…

@dasJ
Copy link
Member

dasJ commented Apr 14, 2022

Doesn't look like it (or I'm holding it wrong) :(

@aforemny aforemny mentioned this pull request Apr 15, 2022
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants