Skip to content

doc: pass nixpkgs-revision instead of nixpkgs#226061

Closed
ghost wants to merge 1 commit intomasterfrom
unknown repository
Closed

doc: pass nixpkgs-revision instead of nixpkgs#226061
ghost wants to merge 1 commit intomasterfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 13, 2023

The doc expressions take both a pkgs packageset as well as a nixpkgs argument whose purpose is unclear.

I traced all the usage of nixpkgs and it appears to be simply "any attrset with a revision attribute". Let's replace this argument with nixpkgs-revision so people know what its purpose is.

I did not change the arguments to doc/default.nix because it might
be considered an exposed public interface (does Hydra
nix-instantiate it directly?). We should give it an entry in
all-packages.nix and access it that way.

The `doc` expressions take both a `pkgs` packageset as well as a
`nixpkgs` argument whose purpose is unclear.

I traced all the usage of `nixpkgs` and it appears to be simply "any
attrset with a `revision` attribute".  Let's replace this argument
with `nixpkgs-revision` so people know what its purpose is.

I did not change the arguments to `doc/default.nix` because it might
be considered an exposed public interface (does Hydra
`nix-instantiate` it directly?).  We should give it an entry in
all-packages.nix and access it that way.
@ghost ghost requested a review from fricklerhandwerk as a code owner April 13, 2023 19:30
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Apr 13, 2023
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Apr 13, 2023
Copy link
Copy Markdown
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

That's probably the right thing to do, but I don't feel qualified to call that shot alone.

@pennae @roberth 👍 👎 ?

@nixos-discourse
Copy link
Copy Markdown

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

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1

@pennae
Copy link
Copy Markdown
Contributor

pennae commented Apr 18, 2023

is this even necessary after #226057? looks like both achieve the same thing.

{ pkgs ? (import ./.. { }), nixpkgs ? { }}:
let
doc-support = import ./doc-support { inherit pkgs nixpkgs; };
doc-support = import ./doc-support { inherit pkgs; nixpkgs-revision = nixpkgs.revision; };
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does this work with [nixpkgs]$ nix-build ./doc?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

No, it doesn't.

I guess there's some ambiguity about what are valid entry points into nixpkgs. Maybe this should wait until that question gets answered.

@roberth
Copy link
Copy Markdown
Member

roberth commented Apr 18, 2023

Keeping nixpkgs around as a convenience method seems like a good thing, but I agree that these functions have no business depending on a whole flake, if that's what you're going for. Not sure how useful such interface segregation is, in this instance.

@ghost
Copy link
Copy Markdown
Author

ghost commented Apr 24, 2023

Keeping nixpkgs around as a convenience method seems like a good thing, but I agree that these functions have no business depending on a whole flake, if that's what you're going for.

That was sort of the idea. I wanted to make the manual callable from situations where you really shouldn't assume the ability to import <nixpkgs> (i.e. from top-level/all-packages.nix).

It turned out that the docs don't use that argument for anything except the revision, so the idea was to pass only that.

@ghost ghost marked this pull request as draft April 24, 2023 04:28
@ghost ghost closed this Oct 22, 2023
@ghost ghost deleted the pr/manual/args branch October 22, 2023 07:38
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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: 0 This PR does not cause any packages to rebuild on Linux.

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

4 participants