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: add stdenv passthru chapter #315909

Merged
merged 4 commits into from
Jun 11, 2024

Conversation

abathur
Copy link
Member

@abathur abathur commented May 30, 2024

Reviewer on another PR queried about documenting a passthru attribute it adds, and looking into it led me to see how thin and scattered existing passthru docs are.

I'm hoping to keep this fairly narrow (reworking the skeleton and moving existing coverage) to focus review on what I see as the big/fundamental questions here:

  • whether we should have such a section
  • whether that section should list common passthru attrs
  • what razors we should apply to decide when a given passthru attr deserves mention here as opposed to a more-specific part of the manual, an in-repo README, code comments, etc.

@NixOS/documentation-team and @infinisil (given ~ownership of repo-internal READMEs, where a large fraction of the more in-depth passthru.tests coverage was moved last year).


Add a 👍 reaction to pull requests you find important.

- These values are not passed to the builder, so you can change them without triggering a rebuild.

One example of the passthru's power is how it enables us to associate the package for an interpreter Python or Ruby with a set of modules built for that specific interpreter (e.g., `python.pkgs` and `ruby.gems`) and to build runtime environments with a collection of those modules (e.g., (`python.withPackages`)[#python.withpackages-function] and `ruby.withPackages`).
<!-- python.pkgs, ruby.gems, and ruby.withPackages are mentioned but don't have dedicated sections to link -->
Copy link
Member Author

Choose a reason for hiding this comment

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

Comment here is just a reminder for why I only linked one of these :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this trying to say that python.pkgs and python.withPackages are defined in python.passthru? If so it should be explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kjeremy Can you clarify how that could be more explicit? (Perhaps in the form of a rephrasing? I feel like the text is already explicitly stating that the passthru is where these are defined.)

Comment on lines 30 to 36
<!--
I'm extracting this statement from the old text because I feel like it isn't very clearly-phrased and doesn't help connect with something most people know already.
I still think we should try to make sure the pattern it models is represented in the list of common attributes, though:

convey a specific dependency of your derivation which contains a program with plugins support.
Later, others who make derivations with plugins can use passed-through dependency to ensure that their plugin would be binary-compatible with built program.
-->
Copy link
Member Author

Choose a reason for hiding this comment

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

Still undecided on this. Roughly a TODO, but I didn't want to go off searching for a real example of this and writing it up if all of this is off-base in the first place.

Comment on lines 40 to 50
Many `passthru` attributes are situational, so this section only attempts to list recurring attributes and patterns.
Very broadly, passthru attributes listed here fall in one of these categories:
<!--
Very broadly, an attribute is worth documenting here if it fits either description below.
Attributes that apply only to a single builder or language ecosystem are best documented alongside the builder or related docs.
-->

- Attributes that are (at least in theory) globally relevant--you could find them on any derivation in nixpkgs (generally because they don't entail any special support be built into the derivation they're added to).
Common examples of this type are `passthru.tests` and `passthru.updateScript`.
- Common patterns for adding extra functionality to a derivation (which do tend to entail building support into the derivation or its passthru).
Common examples of this type are `passthru.optional-dependencies`, `passthru.withPlugins`, and `passthru.withPackages`.
Copy link
Member Author

Choose a reason for hiding this comment

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

I've waffled a little about whether the razor is something the text should try to qualify, or just something we can leave in a comment.

I did a bit of both to force that discussion :)

Copy link
Member Author

Choose a reason for hiding this comment

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

@fricklerhandwerk Could use guidance on this implementation of the razor and whether you think it belongs in the primary text or in a comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the general structure and semantics are fine. I'd do another pass to simplify the language and make it a bit less verbose when you're happy with the current state, but other than that it's a great addition and clarification overall. (A strong indicator of that is that I learned something that was not fully obvious to me beforehand...)

@AndersonTorres
Copy link
Member

This text contains too much excitement for a technical document. It looks like an anime protagonist screaming すごい after each minute.

@fricklerhandwerk
Copy link
Contributor

whether we should have such a section

Absolutely

whether that section should list common passthru attrs

Yes

what razors we should apply to decide when a given passthru attr deserves mention here as opposed to a more-specific part of the manual, an in-repo README, code comments, etc.

If it's commonly used, it should be in the common section. Otherwise the specific build helpers should document their passthrus and refer back to the common section.

Broad strokes:
- create the chapter
- move existing stdenv passthru coverage into it
- move out-of-place coverage of passthru.tests from the stdenv meta
  chapter into it
- (try to) apply 1-sentence-per-line to text I've touched
- add legacy anchors for everything moved
- update existing links to the new anchors
- add tentative motivating text
@abathur
Copy link
Member Author

abathur commented Jun 7, 2024

Force-push to incorporate changes that landed a few hours ago via:

I did reline the changes there and make some small phrasing/consistency edits.

@AndersonTorres
Copy link
Member

What is needed to merge this?

@fricklerhandwerk
Copy link
Contributor

I will properly review and merge on Monday or Tuesday.

Later, others who make derivations with plugins can use passed-through dependency to ensure that their plugin would be binary-compatible with built program.
-->

## Common passthru-attributes {#sec-common-passthru-attributes}
Copy link
Member

Choose a reason for hiding this comment

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

With the new convention, I think this entire section should go into pkgs/README.md instead, because it's only useful to Nixpkgs contributors. Though one could also argue that the attributes are exposed to users, so they could use it, but that's really not the intention behind these.

And it seems like this would combine well with https://github.com/NixOS/nixpkgs/tree/master/pkgs#package-tests. I never got around to doing this in the original PR, but it would be great to continue going that route!

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I see what you mean but I'm not convinced that passthru attributes are purely contributor-facing. As you say those are part of the external interface and I definitely ran into them as a consumer. There may be something to say about how to work with them on the contributor side, but that's a different issue for another time.

Copy link
Member Author

@abathur abathur Jun 7, 2024

Choose a reason for hiding this comment

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

This PR is roughly intended to force this sort of decision. (My primary goal here is that we find clarity on whether and to what extent the passthru should be documented in the manual and/or repo-internal docs and end up with a razor sharp enough to look at a given attr and decide whether/where to document it).

As long as we leave with that razor in hand, I won't be put out if the result is that we close this PR and do something else. (I'm not sure if it will make sense to have a full passthru chapter if, in practice, we think no or very few attrs would be appropriately documented there instead of in-repo.)

CC: @roberth, since this PR results from discussion with him.

abathur and others added 2 commits June 7, 2024 16:48
Make nixpkgs-internal links relative/branchless
razor: if it is only ever needed by contributors, which is likely if links
refer to the latest revision of the source code, then it's for
the contributor guide
@fricklerhandwerk
Copy link
Contributor

@abathur here's a sharper razor: if it is only ever needed by contributors, which is likely if links refer to the latest revision of the source code, then it's for the contributor guide. if examples won't go stale if the underlying code changes or moves, it's for the user's manual.

doc/stdenv/passthru.chapter.md Outdated Show resolved Hide resolved
Comment on lines 40 to 50
Many `passthru` attributes are situational, so this section only attempts to list recurring attributes and patterns.
Very broadly, passthru attributes listed here fall in one of these categories:
<!--
Very broadly, an attribute is worth documenting here if it fits either description below.
Attributes that apply only to a single builder or language ecosystem are best documented alongside the builder or related docs.
-->

- Attributes that are (at least in theory) globally relevant--you could find them on any derivation in nixpkgs (generally because they don't entail any special support be built into the derivation they're added to).
Common examples of this type are `passthru.tests` and `passthru.updateScript`.
- Common patterns for adding extra functionality to a derivation (which do tend to entail building support into the derivation or its passthru).
Common examples of this type are `passthru.optional-dependencies`, `passthru.withPlugins`, and `passthru.withPackages`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the general structure and semantics are fine. I'd do another pass to simplify the language and make it a bit less verbose when you're happy with the current state, but other than that it's a great addition and clarification overall. (A strong indicator of that is that I learned something that was not fully obvious to me beforehand...)

Later, others who make derivations with plugins can use passed-through dependency to ensure that their plugin would be binary-compatible with built program.
-->

## Common passthru-attributes {#sec-common-passthru-attributes}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I see what you mean but I'm not convinced that passthru attributes are purely contributor-facing. As you say those are part of the external interface and I definitely ran into them as a consumer. There may be something to say about how to work with them on the contributor side, but that's a different issue for another time.

doc/build-helpers/images/dockertools.section.md Outdated Show resolved Hide resolved
doc/build-helpers/fetchers.chapter.md Outdated Show resolved Hide resolved
doc/build-helpers/fetchers.chapter.md Outdated Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk merged commit 9ff9bbd into NixOS:master Jun 11, 2024
7 of 8 checks passed
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2024-06-14-documentation-team-meeting-notes-132/47182/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants