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

doc: pass nixpkgs-revision instead of nixpkgs #226061

Closed
wants to merge 1 commit into from
Closed

doc: pass nixpkgs-revision instead of nixpkgs #226061

wants to merge 1 commit into from

Conversation

ghost
Copy link

@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.
Copy link
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

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
Contributor

pennae commented Apr 18, 2023

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

@@ -1,6 +1,6 @@
{ 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
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
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
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
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
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants