execline: wrap unconditionally; strip #78372
Merged
+39
−68
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I don't think there's any situation in which an unwrapped execlineb is useful -- if you want to use different versions of the execlineb tool it'll still prefer ones in PATH. At the same time, implementing the wrapper in this way, as a series of two derivations, meant that we didn't get stdenv goodness for the wrapper. This meant that, for example, the wrapper was not stripped, and so execline ended up with runtime dependencies on gcc and the Linux headers. I don't want to have to reimplement this sort of stuff when it's already in stdenv, and so it makes much more sense to create the wrapper in the mkDerivation call, where all of stdenv's normal magic will find it.
alyssais
commented
Jan 23, 2020
| cc \ | ||
| -O \ | ||
| -Wall -Wpedantic \ | ||
| -D 'EXECLINEB_PATH()="${execline}/bin/execlineb"' \ | ||
| -D 'EXECLINE_BIN_PATH()="${execline}/bin"' \ |
There was a problem hiding this comment.
@Profpatsch why are these functions, btw?
There was a problem hiding this comment.
I don’t remember, but I remember that there was a semantic difference that makes it work/not work
There was a problem hiding this comment.
Consult the gcc manual of your choice I guess :)
ofborg
bot
added
8.has: package (new)
I think there is, if you don’t want to have an overhead of two We don’t have to remove the wrapper, we can use |
|
> I don't think there's any situation in which an unwrapped execlineb is
> useful
I think there is, if you don’t want to have an overhead of two `exec`
calls per call to execlineb (which is about 2–10ms).
I really don't think this is an important use case. If you were worried
about exec overhead, surely you wouldn't be using execline, where
literally every operation adds another exec?
|
|
>> I don't think there's any situation in which an unwrapped execlineb is
>> useful
>
> I think there is, if you don’t want to have an overhead of two `exec`
> calls per call to execlineb (which is about 2–10ms).
I really don't think this is an important use case. If you were worried
about exec overhead, surely you wouldn't be using execline, where
literally every operation adds another exec?
If it really is important, you can always use .execlineb-wrapped. I
certainly don't think it's an important enough use case to warrant its
own option in the execline package. If you're supplying your own
execline tools you can even just add one more symlink to point
"execlineb" in your environment to "${execline}/bin/.execlineb-wrapped".
|
That’s a good counterargument. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
I don't think there's any situation in which an unwrapped execlineb is
useful -- if you want to use different versions of the execlineb tool
it'll still prefer ones in PATH. At the same time, implementing the
wrapper in this way, as a series of two derivations, meant that we
didn't get stdenv goodness for the wrapper. This meant that, for
example, the wrapper was not stripped, and so execline ended up with
runtime dependencies on gcc and the Linux headers. I don't want to
have to reimplement this sort of stuff when it's already in stdenv,
and so it makes much more sense to create the wrapper in the
mkDerivation call, where all of stdenv's normal magic will find it.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)