Mk wrapper fixes #19328
Mk wrapper fixes #19328
Conversation
|
@Profpatsch, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @ttuegel and @fpletz to be potential reviewers. |
doc/stdenv.xml
Outdated
| @@ -1200,7 +1228,7 @@ echo @foo@ | |||
|
|
|||
| <programlisting> | |||
| #! /nix/store/bmwp0q28cf21...-bash-3.2-p39/bin/sh | |||
| PATH=/nix/store/68afga4khv0w...-coreutils-6.12/bin | |||
| //PATH=/nix/store/68afga4khv0w...-coreutils-6.12/bin | |||
There was a problem hiding this comment.
What does the // do or change here?
|
This should be merged to staging together with your other mass rebuild changes. |
| @@ -1,8 +1,28 @@ | |||
| # construct an executable file that wraps the actual executable | |||
There was a problem hiding this comment.
shouldn't documentation like this be kept outside of the scripts to prevent a mass-rebuild? See #16173
There was a problem hiding this comment.
You can bundle changes like this up with other mass rebuilds and you can change the documentation when something in the implementation changes (like it does here)
There was a problem hiding this comment.
Yes, you can. But why would you? If you keep it out, it is also much easier to include it in other documentation, like the manual.
There was a problem hiding this comment.
Because documentation should always be as close a possible to the actual source code. Especially symbol-level documentation.
It won’t be changed until the next time this function is modified, and this is going to trigger a MR anyway.
So the documentation in this PR should be reviewed thoroughly and then no mass rebuilds will be necessary (also multiple mass rebuilds can be bundled together).
Add function documentation to `makeWrapper`. Also add user documentation to the nixpkgs manual.
extraFlagsArray should not be exposed outside of `makeWrapper`, it should only be possible to set it inside a script supplied via the `--run` argument.
Profpatsch
force-pushed
the
mkWrapper-fixes
branch
from
d510b7f
to
5ae1857
Compare
|
Any further comments on this? Otherwise I think we can merge it into staging? |
|
Thanks! I'm sorry such MRs take so long to merge. I have missed the mention unfortunately. (There are too many many sometimes.) |
There is a bug in this feature: It allows extra arguments to leak in from the environment. For example: $ export extraFlagsArray=date $ man ls Note that you get the man page for date rather than for ls. This happens because 'man' happens to use a wrapper (to add groff to its PATH). An attempt to fix this was made in 5ae1857 in PR NixOS#19328 for issue NixOS#2537, but 1. That change didn't actually fix the problem because it addressed makeWrapper's environment during the build process, not the constructed wrapper script's environment after installation, and 2. That change was apparently accidentally lost when merged with 7ff6eec. Rather than trying to fix the bug again, we remove the extraFlagsArray feature, since it has never been used in the public repo in the ten years it has been available. wrapAclocal continues to use its own, separate flavor of extraFlagsArray in a more limited context. The analogous bug there was fixed in 4d7d10d in 2011.
There is a bug in this feature: It allows extra arguments to leak in from the environment. For example: $ export extraFlagsArray=date $ man ls Note that you get the man page for date rather than for ls. This happens because 'man' happens to use a wrapper (to add groff to its PATH). An attempt to fix this was made in 5ae1857 in PR NixOS#19328 for issue NixOS#2537, but 1. That change didn't actually fix the problem because it addressed makeWrapper's environment during the build process, not the constructed wrapper script's environment after installation, and 2. That change was apparently accidentally lost when merged with 7ff6eec. Rather than trying to fix the bug again, we remove the extraFlagsArray feature, since it has never been used in the public repo in the ten years it has been available. wrapAclocal continues to use its own, separate flavor of extraFlagsArray in a more limited context. The analogous bug there was fixed in 4d7d10d in 2011. (cherry picked from commit a45b3ad)
Motivation for this change
fixes #2537
Two commits that are bundled because they both change
make-wrapper.nixand force a mass-rebuild.First one adds documentation to
makeWrapper(also manual).Second one makes
extraFlagsArraylocal as discussed in #2537.