If you call wrapProgram twice on the same program, it breaks

[copumpkin]

It was user error on my part, but as part of some automation, I accidentally called wrapProgram twice on the same program. The first invocation moved the binary foo to .foo-wrapped, and replaced foo with a script that called .foo-wrapped. The second invocation copied the wrapper script over .foo-wrapped and lost the original binary.

It seems like wrapProgram could fail loudly in that case, since I don't think it's ever desirable. Alternately, it could use a better name generation scheme that is collision-aware.

[domenkozar]

Python uses dual wrappers sometimes (which should be refactored), but it works. I remember it's an OSX issue.

[fkz]

I don't think it works under Linux. https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/setup-hooks/make-wrapper.sh#L105:

wrapProgram() {
   local prog="$1"
   local hidden="$(dirname "$prog")/.$(basename "$prog")"-wrapped
   mv $prog $hidden
   makeWrapper $hidden $prog --argv0 '"$0"' "$@"
}

So the mv overwrites the wrapped program. Probably we should fail instead. (Use something like mv -nT $prog $hidden && [ ! -f $prog ]). (Or have I overlooked something?)

[Profpatsch]

wrapProgram is indeed not idempotent (since only one level of wrapping will ever be created). Would be nice to have it patched, or at least for the build to fail if there is already a wrapper.

[Profpatsch]

(triage) maybe it was fixed in the meantime?

[wmertens]

Not fixed yet - on Darwin and Solaris, a shebang can't point to a script. makeWrapper should parse and short circuit wrappers. Or #11133 (comment)

[stale]

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
2. Ask on the NixOS Discourse.
3. Ask on the #nixos channel on irc.freenode.net.

[Profpatsch]

We solved this problem for execline by using a C wrapper, because ELF -> Shebang -> ELF -> Shebang -> … works even on macOS: #71357

Maybe this approach could be generalized, or makeWrapper could always be a binary on macOS.
I’d advise against making it a binary always however, because it hurts debuggability.

[stale]

I marked this as stale due to inactivity. → More info

[wmertens]

Still a problem. I guess this needs an RFC. Ideally, we'd have a wrapper that reads a series of config files (one per wrapping) and then correctly calls the wrapped binary.

This is not always ideal, since scripts are often called as interpreter scriptfile, which breaks if the script isn't using the same interpreter.

So we'd need a wrapper per script type, too.

On top of that, it's not that nice to have the wrapped script/executable live in a separately named file. This can be fixed by putting the wrapper in a separate package that symlinks or hardlinks the rest of the package, but then you get into tradeoffs regarding disk cache, dentry count, and executable image sharing.

Urgh.

[Profpatsch]

I guess you haven’t seen NixOS/rfcs#75 ?

…

On Mon, Dec 7, 2020 at 2:53 PM Wout Mertens ***@***.***> wrote: Still a problem. I guess this needs an RFC. Ideally, we'd have a wrapper that reads a series of config files (one per wrapping) and then correctly calls the wrapped binary. This is not always ideal, since scripts are often called as interpreter scriptfile, which breaks if the script isn't using the same interpreter. So we'd need a wrapper per script type, too. On top of that, it's not that nice to have the wrapped script/executable live in a separately named file. This can be fixed by putting the wrapper in a separate package that symlinks or hardlinks the rest of the package, but then you get into tradeoffs regarding disk cache, dentry count, and executable image sharing. Urgh. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#11586 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAYB5ZR456GOHHPBQXKUXILSTTM5HANCNFSM4BWJAIHA> .

[wmertens]

Oh wow, how did I miss this? That's great. Why don't we close this issue in favor of the RFC?

[Profpatsch]

Let’s wait and see whether the RFC merges, stale bot will remind us :D

[stale]

I marked this as stale due to inactivity. → More info