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

docs: expand explanation of patchShebangs hook #121015

Merged
merged 9 commits into from
Jul 13, 2022

Conversation

fricklerhandwerk
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk commented Apr 28, 2021

  • clarify motivation and mechanism
  • explain usage
  • add interlinks
  • add links to sources to enable research

based on https://discourse.nixos.org/t/what-is-the-patchshebangs-command-in-nix-build-expressions/12656

I haven't yet decided if adding source links to the manual is a good idea and haven't found any discussion on the topic.

  • Pro: Easy entry point for research
  • Con: Will point to old commits eventually

Would be cool if we had a preprocessor that converts references to /path/to/file:L123-L456 in the current repository into GitHub links, but keeping it consistent with changing code would be a can of worms on its own.

Opinions?

Closes #129741

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/what-is-the-patchshebangs-command-in-nix-build-expressions/12656/11

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

Looks great, few minor comments.

doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
@fricklerhandwerk
Copy link
Contributor Author

@jtojnar Thanks for the precise observations, corrected.

@fricklerhandwerk
Copy link
Contributor Author

Anyone wants to merge this?

@siraben
Copy link
Member

siraben commented Jul 31, 2021

LGTM, but please resolve the merge conflict.

@siraben
Copy link
Member

siraben commented Jul 31, 2021

udocker commits here?

@fricklerhandwerk
Copy link
Contributor Author

Random stuff from rebasing, I don't know...

@fricklerhandwerk
Copy link
Contributor Author

fricklerhandwerk commented Mar 19, 2022

@SuperSandro2000 @jtojnar Can we finish the discussion on using source links and merge this? I hope having links in the first place is undisputed, as it helps people diving into the code, and the code is the definition of how things work.

Calling @Ericson2314 because we looked at the PR together.

Some considerations, leading me to conclude that permalinks are the better option:

Pros:

  • permalinks do not break
  • maintenance is optional
  • correctness within documentation ensured without additional cost

Con: (same as for all prose documentation)

  • correctness w.r.t. release version may decay
  • have to be updated manually
    • hopefully by avid boyscout-y readers who notice a reference is old...

One could add an item to the release checklist, to go through permalinks and update them to the latest pre-release revision. That may in fact encourage people to actually look at the docs now and then and see if they still make sense. @tomberek what do you think?

Using links to release branches or master...
Pro:

  • points to most recent version
  • may be fairly stable for many things (but we don't know)

Con:

  • will break eventually
  • must be maintained to keep correctness

@Ericson2314
Copy link
Member

@fricklerhandwerk I would prefer master, because the manual should document the version of the code it itself came from! (In general, with monorepos, there is a tendency to hit issues where the mono repo in affect refers to / depends on prior versions of itself, and I think that is no good!)

The maintenance burden is very real, however, and I don't want to sweap that under the rug. It's a lot of up-front the work, but I think the answer is doing a (very!) technical solution for this:

  • The links are written without reference to github or any branch, in a custom URI syntax of our choosing.
  • Next to the docs are snippits of what we expect those links to point to. Maybe they are named with the URI for easy lookup.

As part of the docs build:

  • A linter that can understand those URLs checks that the text snippit matches the URL
  • A pass on the markdown turns our custom URI into a github URL referring to the right branch (master for unstable docs, release-* for stable docs, etc.)

We thus ensure the docs are in sync with no manual auditing effort, and as a bonus make the release process easier (no need to update the URLs, keep them updated with backports, etc.). Bonus, in addition to links we can have the same method for code snippets that render in the docs too!

This is a lot of work, but I think it is extremely valuable, as it would allow us to document Nixpkgs with far, far more textual references in a way is sustainable with respect to human effort and avoiding the damage of false out of date documentation (arguably worse that no docs at all!)

@fricklerhandwerk
Copy link
Contributor Author

fricklerhandwerk commented Mar 20, 2022 via email

@jtojnar
Copy link
Member

jtojnar commented Mar 20, 2022

I prefer permalinks precisely for the reasons you outlined. As someone who writes docs, the consideration what to do when a link is outdated is most important to me:

  • for master link, I need to find out when the part of manual containing the link was changed, find Nixpkgs commit which master pointed to at that time, look at the link contents and then find the corresponding new place. And I am not even mentioning how to find if the link is incorrect.
  • for permalink, I can simply compare it with master and when the lines the links point to differ, I can look for the old contents (either in tree or in git log -p).

Yes, a new linking scheme such as the one John would resolve this more elegantly. But it is significantly more work than just using the permalink, even though it is essentially the same solution. The main difference is that the new scheme would store the snippet as a part of a link, whereas the permalink scheme stores the snippet in the link target. Yes, the former would allow for more flexibility (e.g. using regexes) but again, I am not sure it is worth the effort.


And yes, all of those are at least partially automatable but until we have such automatization, the permalinks make most sense to me, as mentioned above.

@Ericson2314
Copy link
Member

Ericson2314 commented Mar 20, 2022

Yeah. Links to files from specific commits are fine to start. Maybe we can accumulate a few of those and then the case for the automation will build up over time.

@fricklerhandwerk
Copy link
Contributor Author

@jtojnar @Ericson2314 updated to permalinks. Ready to merge?

@fricklerhandwerk fricklerhandwerk mentioned this pull request Apr 20, 2022
2 tasks
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/stdenv.chapter.md Show resolved Hide resolved
@fricklerhandwerk fricklerhandwerk force-pushed the docs-patch-shebangs branch 2 times, most recently from ffebf47 to b90429d Compare April 20, 2022 19:58
Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
This setup hook patches installed scripts to add Nix store paths to their shebang interpreter as found in the build environment. The [shebang](https://en.wikipedia.org/wiki/Shebang_(Unix)) line tells a Unix-like operating system which interpreter to use to execute the script's contents.

::: note
The [generic builder][generic-builder] populates `PATH` from inputs of the derivation.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe link to the place that actually does that instead, since people might click the link and be confused.

}
```

The file [`patch-shebangs.sh`][patch-shebangs.sh] defines the [`patchShebangs`][patchShebangs] function. It is used to implement [`patchShebangsAuto`][patchShebangsAuto], the [setup hook](#ssec-setup-hooks) that is registered to run during the [fixup phase](#ssec-fixup-phase) by default.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this to the beginning to serve as an overview.

@fricklerhandwerk
Copy link
Contributor Author

@jtojnar Thanks for the hints, incorporated some. Though in general I would like reviewers to tell what must be changed to meet agreed-upon quality standards, not what could be changed because it's nice to have. This PR is already over a year old. We can always slightly improve later if it appears important enough.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Let's merge this soon.

Co-authored-by: Jan Tojnar <jtojnar@gmail.com>
@roberth roberth merged commit 3c1447f into NixOS:master Jul 13, 2022
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-34/20901/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.

patchShebangs docs
7 participants