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: improve fetchers overview, deduplicate readme content, follow doc conventions #297654

Merged
merged 12 commits into from
Apr 3, 2024

Conversation

DanielSidhion
Copy link
Member

@DanielSidhion DanielSidhion commented Mar 21, 2024

Description of changes

This PR starts my work to update and improve the chapter on fetchers in the Nixpkgs manual. It adds more explanations on the overview of the chapter, moves content from the contributor README into the manual, and deduplicates information.

NOTE: this PR started as a change of the fetchers overview AND updating the documentation for fetchurl. After a lot of reviewing, we decided to split this PR into two. The PR concerns the description above. The second PR is #300429 and concerns only with updating the documentation for fetchurl.

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:
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

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

wow that's a lot of documentation!

doc/build-helpers/fetchers.chapter.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
Copy link
Contributor

fricklerhandwerk commented Mar 28, 2024

Started reviewing in docs team meeting today.

This is a rabbit hole, because there is overlapping documentation on updating FOD hashes that ended up in contributor documentation, where it arguably shouldn't be. There are also repetitions of the same mantra in various other places. Ideally we'd merge all of that in the manual's fetcher section, and link to it from everywhere else.

I'll push a few commits to improve the wording on the new stuff, but recommend splitting out the reference documentation in a different PR to get some of that work merged already.

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 28, 2024

@DMills27 and me went all the way and closed the rabbit hole. But it still needs to be split to be reviewable.

@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-03-28-documentation-team-meeting-notes-115/42329/1

doc/build-helpers/fetchers.chapter.md 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
doc/build-helpers/fetchers.chapter.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
doc/build-helpers/fetchers.chapter.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
doc/build-helpers/fetchers.chapter.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
doc/build-helpers/fetchers.chapter.md Outdated Show resolved Hide resolved
doc/build-helpers/fetchers.chapter.md Outdated Show resolved Hide resolved
@DanielSidhion DanielSidhion force-pushed the update-fetchers branch 2 times, most recently from 1fc4499 to 5ae1b53 Compare March 31, 2024 07:07
DanielSidhion and others added 9 commits March 31, 2024 00:09
Co-authored-by: Dominic Mills-Howell <[email protected]>
Co-authored-by: Dominic Mills-Howell <[email protected]>
update wording in the contributors docs

Co-authored-by: Dominic Mills-Howell <[email protected]>
this may be better suited for the `fetchurl` reference, but I'd prefer
just rendering that information into the manual. for now, because
- contributor documentation encourages mirrors
- we can expect contributors to dig into the source
- linking source files is trivial in in-code documentation
we leave it there.

Co-authored-by: Dominic Mills-Howell <[email protected]>
also improve the language a bit, and add links

Co-authored-by: Dominic Mills-Howell <[email protected]>
@DanielSidhion DanielSidhion changed the title doc: improve fetchers overview, add all fetchurl attributes, follow doc conventions doc: improve fetchers overview, deduplicate readme content, follow doc conventions Mar 31, 2024
@DanielSidhion
Copy link
Member Author

@fricklerhandwerk @lolbinarycat: I followed @fricklerhandwerk's suggestion and split this PR into two. Did a bit of git history rewriting to get only the relevant changes in this PR, and #300429 has the fetchurl changes.

@fricklerhandwerk - I made some changes and reworded some stuff on top of your changes, please take a look at the comments from my review to understand the reasoning. Requesting your review and @lolbinarycat's and I'm hoping we can agree on a merge soon 😊

Thank you all for the reviews, I feel this has turned out much better with your involvement!

doc/build-helpers/fetchers.chapter.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
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.

Just a few small things. The other changes are clear improvements, thanks a lot!

doc/build-helpers/fetchers.chapter.md Outdated Show resolved Hide resolved
Copy link
Contributor

@lolbinarycat lolbinarycat left a comment

Choose a reason for hiding this comment

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

look pretty good but i think there are some slight improvements that can be made.

doc/build-helpers/fetchers.chapter.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
pkgs/README.md Outdated Show resolved Hide resolved
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.

This has probably seen enough churn now. Let's fix forward if rough edges remain.

@fricklerhandwerk fricklerhandwerk merged commit 0decb32 into NixOS:master Apr 3, 2024
21 checks passed
@DanielSidhion DanielSidhion deleted the update-fetchers branch April 3, 2024 06:57
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.

5 participants