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

nixpkgs-manual: init #311459

Merged
merged 10 commits into from
Jul 26, 2024
Merged

nixpkgs-manual: init #311459

merged 10 commits into from
Jul 26, 2024

Conversation

philiptaron
Copy link
Contributor

@philiptaron philiptaron commented May 13, 2024

Motivation

Make it easier and more natural to build the contents of the doc/ directory.

Description of changes

  • Reshape the derivations from doc/ to callPackage style.
  • When needed, extract a derivation into its own file.
  • Organize them under the nixpkgs-manual name.
  • Make doc/default.nix and doc/shell.nix forward to them.

The diff is pretty chunky, but it's not too bad when you have the old and new side by side... sadly GitHub makes this pretty impossible.

Incidental changes:

  • nixfmt on new files
  • stdenvNoCC instead of stdenv as appropriate
  • Derivations accessible through passthru

Why not pkgs/by-name?

Because it has a rule that packages in pkgs/by-name cannot refer to files outside their own directory, which this obviously needs to do. I've opened NixOS/nixpkgs-vet#64 to ideate about this use case -- it's not broad, but it would be nice -- but in the meantime, this derivation can't use pkgs/by-name.

The CI failure says that merging won't break the main branch.

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:
    • nix-build doc/
    • nix-build -A nixpkgs-manual
    • pushd doc/ && nix-shell
    • On master, nix-build doc/, on branch nix-build doc/, and then diffoscope <original> <new> (no changes except nix-support/hydra-build-products)
  • 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/)
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label May 13, 2024
@philiptaron philiptaron marked this pull request as draft May 13, 2024 20:23
@philiptaron philiptaron requested a review from infinisil May 13, 2024 20:41
@philiptaron

This comment was marked as outdated.

@philiptaron

This comment was marked as outdated.

Copy link
Contributor

@hsjobeki hsjobeki left a comment

Choose a reason for hiding this comment

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

Maybe we should not add the docs with pkgs-by-name, but via an overlay or add an exclude from checks since it will always import files from /doc ?

doc/default.nix Show resolved Hide resolved
pkgs/by-name/ni/nixpkgs-manual/options-json.nix Outdated Show resolved Hide resolved
@philiptaron
Copy link
Contributor Author

Maybe we should not add the docs with pkgs-by-name, but via an overlay or add an exclude from checks since it will always import files from /doc ?

I've put this PR in draft mode while figuring out the way forward with nixpkgs-check-by-name rules. No such exclusion mechanism exists today, and there's a real use case for it here, I think.

I'm going to have better availability to contribute to open source during the summer. That's when I plan on re-engaging with this.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 4, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 15, 2024
@philiptaron philiptaron marked this pull request as ready for review July 16, 2024 02:58
@philiptaron philiptaron requested review from hsjobeki and a team July 16, 2024 02:58
@philiptaron
Copy link
Contributor Author

@hsjobeki said:

Maybe we should not add the docs with pkgs-by-name, but via an overlay or add an exclude from checks since it will always import files from doc/?

This is the direction I went. The current nixpkgs-check-by-name failure says this:

- Attribute `pkgs.nixpkgs-manual` is a new top-level package using `pkgs.callPackage ./pkgs/data/documentation/nixpkgs-manual { /* ... */ }`.
  Please define it in pkgs/by-name/ni/nixpkgs-manual/package.nix instead.
  See `pkgs/by-name/README.md` for more details.
  Since the second `callPackage` argument is `{ }`, no manual `callPackage` in pkgs/top-level/all-packages.nix is needed anymore.

This PR introduces additional instances of discouraged patterns as listed above.
Merging is discouraged but would not break the base branch.

This is now ready for review, I think.

@philiptaron
Copy link
Contributor Author

There is one thing I could do that would satisfy nixpkgs-check-by-name and the existing code: move everything, web assets and all, into pkgs/by-name/ni/nixpkgs-manual and make a symlink to that directory replacing the doc/ directory.

@philiptaron philiptaron marked this pull request as draft July 16, 2024 04:55
@philiptaron

This comment was marked as outdated.

@fricklerhandwerk
Copy link
Contributor

Re-doing the construction to be callPackage style is defintely a good thing, but I fear that hiding the sources in pkgs will make it pretty hard to find. The manual is pretty important and keeping it in /doc would be vital to make it as likely as possible for people to find it on their own.

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Jul 26, 2024

The manual is pretty important and keeping it in /doc would be vital to make it as likely as possible for people to find it on their own.

Can we add a symlink? (doc/nixpkgs-manual -> pkgs/.../nixpkgs-manual) Even the GH web ui can follow symlinks.

@philiptaron
Copy link
Contributor Author

The manual is pretty important and keeping it in /doc would be vital to make it as likely as possible for people to find it on their own.

Can we add a symlink? (doc/nixpkgs-manual -> pkgs/.../nixpkgs-manual) Even the GH web ui can follow symlinks.

This it what I suggested in #311459 (comment) -- and yeah, it's quite possible, I believe.

@github-actions github-actions bot added the 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS label Jul 26, 2024
@philiptaron philiptaron marked this pull request as ready for review July 26, 2024 21:19
@infinisil
Copy link
Member

You have my blessing to ignore the pkgs/by-name check failure! This kind of failure won't break master, and it's not worth trying to work around it when it's such a special case.

The real solution imo is to get away from the messy single pkgs namespace, which currently contains both packages, but also a whole lot else. But that's for another day :)

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I reviewed all commits, looking good except one thing!

Comment on lines +18 to +22
assert lib.hasPrefix root declStr;
{
url = "https://github.com/NixOS/nixpkgs/blob/master/${subpath}";
name = subpath;
};
Copy link
Member

Choose a reason for hiding this comment

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

I noticed you changed this to an assert when it was this before:

nixpkgs/doc/default.nix

Lines 89 to 93 in 6a03708

if hasPrefix (toString ../..) (toString decl)
then
let subpath = removePrefix "/" (removePrefix (toString ../.) (toString decl));
in { url = "https://github.com/NixOS/nixpkgs/blob/master/${subpath}"; name = subpath; }
else decl)

Notably I'd expect this to break manual rendering when building non-Nixpkgs modules using documentation.nixos.includeAllModules

Copy link
Contributor Author

@philiptaron philiptaron Jul 26, 2024

Choose a reason for hiding this comment

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

Yes! I thought that for the nixpkgs manual, there wouldn't be ... non-nixpkgs modules. Am I wrong there?

Notably this would be terribly incorrect for the NixOS manual.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yes you're right! Mixed things up here :)

Comment on lines +18 to +22
assert lib.hasPrefix root declStr;
{
url = "https://github.com/NixOS/nixpkgs/blob/master/${subpath}";
name = subpath;
};
Copy link
Member

Choose a reason for hiding this comment

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

Ah yes you're right! Mixed things up here :)

@infinisil infinisil merged commit d2a2dbc into NixOS:master Jul 26, 2024
25 of 31 checks passed
@philiptaron
Copy link
Contributor Author

Thank you as always @infinisil!

@Artturin
Copy link
Member

This shows up in the rebuild count by ofborg in unrelated packages https://gist.github.com/GrahamcOfBorg/e02bf2c3d2c6a52e1bfb155f6462a9a4

@philiptaron
Copy link
Contributor Author

This shows up in the rebuild count by ofborg in unrelated packages https://gist.github.com/GrahamcOfBorg/e02bf2c3d2c6a52e1bfb155f6462a9a4

I'll be able to take a look tomorrow. Likely something is taking a dependency on the whole Nixpkgs tree somewhere.

@philiptaron
Copy link
Contributor Author

philiptaron commented Jul 29, 2024

OK, I root-caused this rebuild. Step to repro, in the nixpkgs root:

git checkout master
nix-build -A nixpkgs-manual
mv result result-on-master
git commit --allow-empty -m "testing"
nix-build -A nixpkgs-manual
nix-diff ./result-on-master ./result

Here's the diff:

- ./result-on-master:{out}
+ ./result:{out}
• The environments do not match:
    buildPhase=''
        /nix/store/k26j1ix7r2sj83f7rdrjjnsw3gb0zfyi-documentation-highlighter/loader.js

      cp -t out ./style.css ./anchor.min.js ./anchor-use.js

      nixos-render-docs manual html \
        --manpage-urls ./manpage-urls.json \
    -   --revision f4f322d1424aa547eba9cb092f905f5ceb9b639c \
    +   --revision 65abf7019bd37b6ad20ea008843ab4a9d316eaa5 \
        --stylesheet style.css \
        --stylesheet highlightjs/mono-blue.css \
        --script ./highlightjs/highlight.pack.js \
        --script ./highlightjs/loader.js \
''

The revision is being incorporated with these lines:

nixos-render-docs manual html \
--manpage-urls ./manpage-urls.json \
--revision ${lib.trivial.revisionWithDefault (nixpkgs.rev or "master")} \
--stylesheet style.css \
--stylesheet highlightjs/mono-blue.css \

lib.trivial.revisionWithDefault has this implementation:

nixpkgs/lib/trivial.nix

Lines 437 to 445 in 4089e29

revisionWithDefault =
default:
let
revisionFile = "${toString ./..}/.git-revision";
gitRepo = "${toString ./..}/.git";
in if lib.pathIsGitRepo gitRepo
then lib.commitIdFromGitRepo gitRepo
else if lib.pathExists revisionFile then lib.fileContents revisionFile
else default;

Which makes the whole thing make sense. Every git revision makes this rebuild, because the documentation includes the git revision it was built with.

No other package includes the git revision it was built with in nixpkgs; the other usage is in nixos/modules/misc/version.nix, which uses it as a default for options.system.revision.

@philiptaron philiptaron deleted the doc-in-pkgs branch July 29, 2024 18:13
@philiptaron
Copy link
Contributor Author

My proposed fix is here:

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 8.has: package (new) This PR adds a new package 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.

9 participants