makeWrapper: accept `--argv0` flag #9562

ttuegel · 2015-08-30T16:01:42Z

This flag allows setting the argv[0] parameter given to the wrapped executable, in case the new argv[0] passthrough functionality is not desired. The argv[0] parameter can be given explicitly, or the old behavior can be recovered by passing an empty argv[0], i.e. makeWrapper --argv0 "" ....

This would rebuild 7514 packages.

We need this for a proper fix for a Conkeror bug. There is a workaround which I will apply if this patch is rejected from the release-15.09 branch. If accepted, I would also merge this into master.

ttuegel · 2015-08-30T16:02:46Z

/cc @domenkozar

domenkozar · 2015-08-30T20:49:43Z

+1 from me

vcunat · 2015-08-30T21:20:09Z

I think that by default we might want to keep $0 unchanged when passing through the wrapper, although there's probably very few places where it matters. AFAIK the only larger group of $0 users are those utilities that change behavior depending on what symlink runs them.

ttuegel · 2015-08-30T21:35:19Z

I think that by default we might want to keep $0 unchanged when passing through the wrapper, although there's probably very few places where it matters. AFAIK the only larger group of $0 users are those utilities that change behavior depending on what symlink runs them.

That's right; the current default is for the wrapper to pass argv[0] through, and this patch doesn't do anything to change that. The problem is that when you wrap Firefox, the wrapper must set argv[0]=/nix/store/...-firefox-.../bin/firefox so it can find its runtime dependencies.

vcunat · 2015-08-30T21:41:19Z

Well, by "unchanged" (during wrapping) I meant what firefox needs – i.e. if you wrap a symlink, you will still get the original $0 and not some .foo-wrapped.

ttuegel · 2015-08-30T22:02:21Z

Well, by "unchanged" (during wrapping) I meant what firefox needs – i.e. if you wrap a symlink, you will still get the original $0 and not some .foo-wrapped.

I'm still not sure we're on the same page here. In a call makeWrapper $old $new, is $old what you mean when you say "original $0"? To me, the "original $0" is the $0 that was assigned by the shell at runtime, which is usually the same as $old, unless you intentionally invoked with exec -a (in which case I think the wrapper should not override your intentional choice). The timeline of this feature is this:

Prior to c234f37, calling makeWrapper $old $new sets $0=$old, so that calls to wrapProgram foo sets $0=/nix/store/.../.foo-wrapped. We all agree this behavior is bad.
After that revision, wrapped executables are always invoked with the same argv[0] that the wrapper was invoked with. Note that $0 is still $old unless you intentionally used exec -a (which the wrapper should respect!), so nothing breaks unless the package relied on the old (broken) argv[0]=/nix/store/.../.foo-wrapped behavior.
Firefox really does rely on the broken behavior. (Actually, it just needs the prefix of the path, it doesn't seem to care about basename $0.)

So, I'm not sure what your position is on this feature.

vcunat · 2015-08-31T07:17:40Z

To be short, I agree with merging, but my strongest point was addressed by c234f37 already.

I mistakenly thought that firefox problem is about matching basename and not the dirname, as I hadn't read all this properly. I'm sorry for the confusion.

vcunat · 2015-08-31T07:21:18Z

The main problem in all this is IMHO that the content/meaning of $0 is rather under-specified.

lucabrunox · 2015-08-31T08:03:21Z

pkgs/build-support/setup-hooks/make-wrapper.sh

    done

    # Note: extraFlagsArray is an array containing additional flags
    # that may be set by --run actions.
-    echo exec -a '"$0"' "$original" $flagsBefore '"${extraFlagsArray[@]}"' '"$@"' >> $wrapper
+    echo exec ${argv0:+-a} $argv0 "$original" \


I don't understand the $argv0 conditional here. If argv0 is empty, it will exec "" $original, right? Shouldn't -a be there always?

No, in fact, that second $argv should probably be in the conditional. However, another issue was brought to my attention, which I will address in the comments.

ttuegel · 2015-08-31T10:58:09Z

As @vcunat said, the semantics of argv[0] is pretty poorly defined!

I said something above that I realize now is not quite right. If we call makeWrapper with

makeWrapper $old $new ...

then the argv[0] that is passed through is $new, not $old. Now, if makeWrapper is being invoked through wrapProgram, i.e. if we are wrapping a program "in-place", this is what we want. But, we have many "utility" wrappers in Nixpkgs where the intent is not to replace a program with its wrapped version, but to provide some extra function. For example, Conkeror wrapping Firefox. Another example is my very own emacsWithPackages, which the original argv[0] patch (embarrassingly) breaks!

To better address the most common uses of makeWrapper versus wrapProgram, I am going to:

revert to the old default makeWrapper behavior: don't set argv[0] at all, which has the effect of setting argv[0]=$old, and
make wrapProgram add an --argv0 flag to pass-through argv[0].

I think these options are what most people will want when they call makeWrapper versus wrapProgram. As evidence, I cite the fact that neither Firefox, nor emacsWithPackages will need to be patched if I implement this default behavior.

Comments? Objections?

vcunat · 2015-08-31T11:05:11Z

Yes, this new proposal certainly seems the best option so far :-)

By default `makeWrapper` will not set argv[0] (this is a reversion to the old default behavior). Based on the breakage we have seen from changing the default, this is what most people want. The `wrapProgram` function will send `--argv0 '"$0"'` to `makeWrapper`, i.e. it will continue to pass-through the argv[0] that the wrapper is called with.

Not strictly necessary, but Firefox must be called with exactly this argv[0] every time, so there is no reason to admit any other option.

ttuegel · 2015-08-31T11:16:22Z

I changed my mind about Firefox: it doesn't need to be patched now, but we know it needs to be called with a specific argv[0], so there's no reason to allow any pass-through.

vcunat · 2015-09-01T08:29:11Z

BTW, gtk3-enabled firefox won't start for me on current 15.09 with Could not find the Mozilla runtime., but we don't really need to hurry because of this non-default setting.

vcunat · 2015-09-01T10:53:40Z

pkgs/applications/networking/browsers/firefox/default.nix

@@ -99,6 +99,7 @@ stdenv.mkDerivation rec {
    '' + lib.optionalString enableGTK3
    ''
      wrapProgram "$out/bin/firefox" \
+        --argv0 "$out/bin/firefox" \  # argv[0] must point to firefox itself


The comment ruins the backslash ;-)

And even when I fix it, this wrapper doesn't work for me. It seems like the .firefox-wrapped basename is important, after all.

By default `makeWrapper` will not set argv[0] (this is a reversion to the old default behavior). Based on the breakage we have seen from changing the default, this is what most people want. The `wrapProgram` function will send `--argv0 '"$0"'` to `makeWrapper`, i.e. it will continue to pass-through the argv[0] that the wrapper is called with.

vcunat · 2015-09-01T11:28:36Z

I pushed the general --argv0 change to master, along with a security mass-rebuild commit. For the firefox change, more is needed anyway: #9368 (comment)

Also add a simple test detecting such problems.

ttuegel · 2015-09-01T21:33:54Z

@vcunat If that commit fixes Firefox, shall we push it to stable with the --argv0 patch?

By default `makeWrapper` will not set argv[0] (this is a reversion to the old default behavior). Based on the breakage we have seen from changing the default, this is what most people want. The `wrapProgram` function will send `--argv0 '"$0"'` to `makeWrapper`, i.e. it will continue to pass-through the argv[0] that the wrapper is called with. (cherry picked from commit 61cad61)

Also add a simple test detecting such problems. (cherry picked from commit f65b692)

vcunat · 2015-09-02T04:21:10Z

Pushed. I was waiting for more mass-rebuild changes to push together to 15.09.

By default `makeWrapper` will not set argv[0] (this is a reversion to the old default behavior). Based on the breakage we have seen from changing the default, this is what most people want. The `wrapProgram` function will send `--argv0 '"$0"'` to `makeWrapper`, i.e. it will continue to pass-through the argv[0] that the wrapper is called with. (cherry picked from commit 61cad61)

Also add a simple test detecting such problems. (cherry picked from commit f65b692)

ttuegel added 0.kind: bug Something is broken 0.kind: enhancement Add something new 1.severity: mass-rebuild This PR causes a large number of packages to rebuild labels Aug 30, 2015

ttuegel added this to the 15.09 milestone Aug 30, 2015

lucabrunox reviewed Aug 31, 2015
View reviewed changes

ttuegel force-pushed the make-wrapper branch from 190fa13 to c1af8a3 Compare August 31, 2015 11:07

ttuegel force-pushed the make-wrapper branch from c1af8a3 to 3fa41bb Compare August 31, 2015 11:08

firefox: set argv[0] explicitly in wrapper

6d01158

Not strictly necessary, but Firefox must be called with exactly this argv[0] every time, so there is no reason to admit any other option.

vcunat reviewed Sep 1, 2015
View reviewed changes

vcunat added a commit that referenced this pull request Sep 1, 2015

firefox: fix argv0 with enableGTK3 (/cc #9562)

f65b692

Also add a simple test detecting such problems.

vcunat added a commit that referenced this pull request Sep 2, 2015

firefox: fix argv0 with enableGTK3 (/cc #9562)

134b4b4

Also add a simple test detecting such problems. (cherry picked from commit f65b692)

vcunat closed this Sep 2, 2015

vcunat mentioned this pull request Sep 2, 2015

The Eclipse executable launcher was unable to locate its companion shared library #9587

Closed

thomas-morgan mentioned this pull request Oct 8, 2015

LilyPond #10290

Closed

adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024

firefox: fix argv0 with enableGTK3 (/cc NixOS#9562)

a26a27f

Also add a simple test detecting such problems. (cherry picked from commit f65b692)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makeWrapper: accept `--argv0` flag #9562

makeWrapper: accept `--argv0` flag #9562

ttuegel commented Aug 30, 2015

ttuegel commented Aug 30, 2015

domenkozar commented Aug 30, 2015

vcunat commented Aug 30, 2015

ttuegel commented Aug 30, 2015

vcunat commented Aug 30, 2015

ttuegel commented Aug 30, 2015

vcunat commented Aug 31, 2015

vcunat commented Aug 31, 2015

lucabrunox Aug 31, 2015

ttuegel Aug 31, 2015

ttuegel commented Aug 31, 2015

vcunat commented Aug 31, 2015

ttuegel commented Aug 31, 2015

vcunat commented Sep 1, 2015

vcunat Sep 1, 2015

vcunat Sep 1, 2015

vcunat commented Sep 1, 2015

ttuegel commented Sep 1, 2015

vcunat commented Sep 2, 2015

makeWrapper: accept --argv0 flag #9562

makeWrapper: accept --argv0 flag #9562

Conversation

ttuegel commented Aug 30, 2015

ttuegel commented Aug 30, 2015

domenkozar commented Aug 30, 2015

vcunat commented Aug 30, 2015

ttuegel commented Aug 30, 2015

vcunat commented Aug 30, 2015

ttuegel commented Aug 30, 2015

vcunat commented Aug 31, 2015

vcunat commented Aug 31, 2015

lucabrunox Aug 31, 2015

Choose a reason for hiding this comment

ttuegel Aug 31, 2015

Choose a reason for hiding this comment

ttuegel commented Aug 31, 2015

vcunat commented Aug 31, 2015

ttuegel commented Aug 31, 2015

vcunat commented Sep 1, 2015

vcunat Sep 1, 2015

Choose a reason for hiding this comment

vcunat Sep 1, 2015

Choose a reason for hiding this comment

vcunat commented Sep 1, 2015

ttuegel commented Sep 1, 2015

vcunat commented Sep 2, 2015

makeWrapper: accept `--argv0` flag #9562

makeWrapper: accept `--argv0` flag #9562