makeBinaryWrapper: create binary wrappers #95569

FRidh · 2020-08-16T09:32:06Z

When packaging programs we often need to change the environment the program is run in.
Typically this is done using wrappers. Thus far, the wrappers produced were typically
shell scripts, thus interpreted, and with a shebang. This is an issue on e.g. Darwin,
where shebangs cannot point at interpreted scripts.

This commit introduces a new program for creating binary wrappers. The program, written
currently in Python, produces an instruction in JSON which is loaded into a Nim program
and then compiled into a binary.

With interpreted wrappers one can easily check the code to see what it is doing, but with
a binary that is more difficult. To that end, an environment variable NIX_DEBUG_WRAPPER
is introduced which, when set, causes an executed wrapper to print the embedded JSON
instead of exec'ing into the wrapped executable.

The Python program aims to be compatible with the existing makeWrapper but is lacking
features, mostly because of difficulties with implementing them using argparse.

Relevant issues

To do

documentation
tests
check cross-compilation
fix prefix/suffix

Things done

FRidh · 2020-08-16T09:33:40Z

I would have preferred writing the Python part in Nim as well, but did not because I dislike using optparse-like parsing. However, it seems if we want to achieve full compatibility with makeWrapper there is really no choice.

Note the wrappers are around 83 kB. It takes about 1.6 seconds wall clock time to produce a wrapper.

FRidh · 2020-08-16T09:35:29Z

pkgs/build-support/make-binary-wrapper/src/lib/libwrapper/wrapper.nim

+    let wrapper = to(wrapperDescription, Wrapper)
+    processEnvironment(wrapper.environment)
+    let argv = wrapper.original # convert target to cstring
+    let argc = allocCStringArray(wrapper.flags)


flags don't work yet

FRidh · 2020-08-16T09:38:33Z

pkgs/build-support/make-binary-wrapper/src/lib/libwrapper/wrapper.nim

+
+# Wrapper type as used by the wrapper-generation code as well in the actual wrapper.
+
+type


The JSON created for a wrapper needs to follow this exact structure.

doronbehar

Looks Good. But I'm not sure I fully figured out what is the final result - is it a python depending executable?

doronbehar · 2020-08-16T09:47:42Z

pkgs/build-support/make-binary-wrapper/src/lib/libwrapper/wrapper.nim

+type
+    SetVar* = object
+        variable*: string
+        value*: string


I'd like it to be possible to SetVar multiple values from an array in wrapper.json.

The way it is now you can have

"environment": { "set": [ { "variable": "HELLO", "value": "foo" }, { "variable": "HELLO", "value": "bar" } ] }

Thus, order will matter.

Am I correct you are suggesting you want the following?

"environment": { "set": [ { "variable": "HELLO", "values": [ "foo", "bar" ] } ] }

Yes I want it to be an array, and also there should be a separator key as well, because some env vars use e.g ; and not :. : should be the default though.

Am I correct to understand there's both SetVar and environment.set?

environment.set is a sequence of SetVar objects where SetVar has the fields variable and value.

I see what you're after. What I did here is basically take the makeWrapper arguments as is. If you want export HELLO="foo:bar" then in case of makeWrapper write --set HELLO "foo:bar", and so you have variable HELLO and value foo:bar. In that case one does not need to know what separator is used.

doronbehar · 2020-08-16T10:24:45Z

So I understand that makeBinaryWrapper is a shell command that should be accessible just like makeWrapper and wrapProgram. If so, the whole design looks too complicated to me, at least for the purpose of declarative wrappers' current POC design (#85103).

There, I calculated the arguments to makeWrapper, but a significant part of the whole idea is to create a separate derivation of a wrapper that will call the unwrapped executable from it's original drv. What if we'd implement / integrate this binary wrapper creator as a Nix builder, and not as a shell command available during a build? It should make your life easier since you could read and write the JSON wrap data via Nix and not Python (assuming I figured out your design). Plus, it should enable you to construct the Nim code or whatever you'd choose to be compiled as the wrapper, via Nix replacement during build.

Please note that another significant advantage of using a separate derivation (examples include #88620 & #88110) for a wrapper is the reduce of rebuilds due to a request to change the environment.

I also think we should be able to get along with C as the language with which we'll compile the wrapper - that's more minimal but considering most of us have stdenv in our closure and we don't have Nim, it may come about quicker to edit wrapper derivations if building them only requires stdenv.

What I imagine as a binary wrapper creator, is a Nix builder that writes during build a super dumb C program with env vars given by a Nix. Super roughly:

let
  # overly simplified
  env.NIX_PYTHONPATH = "$out/${python.sitePackages}";
in
  buildCommand = ''
    echo 'setenv("NIX_PYTHONPATH", ${env.NIX_PYTHONPATH})' > wrapper.c
    echo 'exec("${placeholder "out"}/bin/.unwrapped")' > wrapper.c
    gcc wrapper.c -o $out/bin/wrapper
  '';

EDIT1: Edited nix expression to be a bit more real.
EDIT2: Naturally, for more complex env settings such as what --prefix implements, a bit more complex getenv and then setenv should be used I guess...

FRidh · 2020-08-16T10:43:50Z

The Nim part is the actual wrapper code. It could have been C, but I find Nim much easier to work with. It's also lightweight and fast so no issue regarding bootstrapping. Closure size is slightly larger than just the C compiler, but the difference is small (36 MB).

The Python part invokes the compilation of the binary wrapper. Some code needs to invoke the compiler, and that's done there now. It also checks the instruction.

The Python part also offers the makeWrapper program which aims to be compatible with the non-binary wrapper creation shell function makeWrapper. Next step is to introduce another executable that takes a JSON instruction for a set of wrappers to be created, the idea here being that hooks have created that JSON instruction.

So I understand that makeBinaryWrapper is a shell command that should be accessible just like makeWrapper and wrapProgram. If so, the whole design looks too complicated to me, at least for the purpose of declarative wrappers' current POC design (#85103).

This is about creating the actual wrappers, not on how we from a derivation, instruct how they should be created. This is solely to get a binary wrapper built. Declarative wrappers is a separate part that is further on top, which eventually leads to the JSON instruction describing all wrappers to be build.

What I imagine as a binary wrapper creator, is a Nix builder that writes during build a super dumb C program with env vars given by a Nix. Super roughly:

In any derivation you will have typically more than one wrapper that needs to be created. Each of these wrappers may need a different instruction.

doronbehar · 2020-08-16T11:14:13Z

In any derivation you will have typically more than one wrapper that needs to be created. Each of these wrappers may need a different instruction.

I understand. But still, even with different environment and args to different executables, it's all about constructing the right buildCommand - You can instruct what wrapping do be done either via Nix, before buildCommand is evaluated, or you can do that with makeBinWrapper at the buildCommand itself.

Declarative wrappers is a separate part that is further on top, which eventually leads to the JSON instruction describing all wrappers to be build.

Indeed, in it's current design, this PR shouldn't necessarily relate or depend upon declarative wrappers and this fact might help it reach reception easier, especially due to having the same interface as makeWrapper which is already well known. But, I still think a Nix builder constructing a proper buildCommand is better, although such builder's usage might intimidate casual contributors, due to the lack of Nix programming experience in the community.

When packaging programs we often need to change the environment the program is run in. Typically this is done using wrappers. Thus far, the wrappers produced were typically shell scripts, thus interpreted, and with a shebang. This is an issue on e.g. Darwin, where shebangs cannot point at interpreted scripts. This commit introduces a new program for creating binary wrappers. The program, written currently in Python, produces an instruction in JSON which is loaded into a Nim program and then compiled into a binary. With interpreted wrappers one can easily check the code to see what it is doing, but with a binary that is more difficult. To that end, an environment variable `NIX_DEBUG_WRAPPER` is introduced which, when set, causes an executed wrapper to print the embedded JSON instead of exec'ing into the wrapped executable. The Python program aims to be compatible with the existing `makeWrapper` but is lacking features, mostly because of difficulties with implementing them using `argparse`.

For `makeBinaryWrapper` we preferably keep the closure small so it can be used early on during bootstrapping.

FRidh · 2020-08-16T15:53:00Z

pkgs/development/compilers/nim/default.nix

-  boehmgc, sfml, sqlite }:
+{ stdenv, lib, fetchurl, makeWrapper, openssl, pcre, readline
+, boehmgc, sfml, sqlite
+, minimal ? false


Cross-compiling the minimal nim is currently broken. I think I went to far with dropping dependencies.

Using the Nim compiler for Nixpkgs cross-compilation is broken generally. We need to use a wrapper with compiler, but at least we can reuse a single buildPackages compiler binary for different host platforms. I will make a PR.

andrew-d · 2020-08-21T22:48:10Z

FWIW, I'm pretty 👎 on introducing another language into a core part of Nixpkgs like this, especially one that is fairly niche like Nim - presumably this means that it would need to be supported/ported/etc. on new platforms, in addition to what we already support.

If Nim is indeed that much easier to use (haven't tried it personally!), what do you think about instead checking in the generated .c files? That way you can continue to use Nim (or whatever) to generate them, but we won't need to depend on it at build time.

ehmry · 2020-08-22T07:03:34Z

pkgs/development/compilers/nim/default.nix

+    "-lpcre"
+    "-lreadline"
+    "-lsqlite3"
+  ]);


This doesn't look right to me, if these libraries aren't linked into the compiler explicitly the compiler will still potentially load the libraries at runtime with dlopen. Is it possible to leave them in or does that create a dependency loop? Ref #95692

As soon as we would use it to replace makeWrapper it would create a dependency loop. Now, we deal with those already during bootstrapping, but if we can avoid it that would be good.

I prefer to get the build-time closure down in size because that's more convenient during bootstrapping, however, all of these packages are built very early on already anyway.

FWIW sfml is not needed, I think thats an artifact from when we ran the compiler and stdlib tests.

ehmry · 2020-08-22T07:10:26Z

pkgs/build-support/make-binary-wrapper/src/lib/libwrapper/wrapper.nim

+    echo(jsonString)
+else:
+    # Unfortunately parsing JSON during compile-time is not supported.
+    let wrapperDescription = parseJson(jsonString)


I think the Npeg library could parse the environment at compile time - https://github.com/zevv/npeg

… or just generate Nim code directly?

Could you elaborate what you mean with generating Nim code here directly? It's important that a generated wrapper does what it should do, but that we can also still check how it was configured.

I mean generating a const Nim tuple of arrays rather than JSON. I was just thinking that the wrapper shouldn't neeed to do much heap allocation.

Profpatsch · 2020-09-09T10:31:02Z

Can you give some insight in why you chose nim over, like C/C++/rust/go for this rather base feature?
As far as I know we don’t have any nim code in any stdenv-related building blocks so far, so this adds another bootstrapping requirement to nixpkgs.

FRidh · 2020-09-09T10:54:20Z

Can you give some insight in why you chose nim over, like C/C++/rust/go for this rather base feature?
As far as I know we don’t have any nim code in any stdenv-related building blocks so far, so this adds another bootstrapping requirement to nixpkgs.

I want to solve an issue that is recurring using the tools and knowledge I have or can use. For my other projects I favor Nim over the others, and I also think in Nixpkgs it would be very valuable (mostly because of NimScript). For in Nixpkgs I don't care what language this functionality is implemented in, although it would for sure be beneficial if more are familiar with it. I won't work with the other languages myself though because I don't have the time for that.

Regarding makeBinaryWrapper, I am actually thinking of just having it in a separate repo and fetching it like we do any other tool. There is no reason for this to be Nixpkgs-specific. See also patchelf.

ehmry · 2020-09-09T10:56:18Z

Nim may also be more portable (or cheaper to maintain portability) than Rust or Go, because the runtime is simple and stable.

Profpatsch · 2020-09-09T11:07:36Z

With interpreted wrappers one can easily check the code to see what it is doing, but with
a binary that is more difficult. To that end, an environment variable NIX_DEBUG_WRAPPER
is introduced which, when set, causes an executed wrapper to print the embedded JSON
instead of exec'ing into the wrapped executable.

I like this idea, why not exec nonetheless? Sounds like it would be hard to get the trace of exactly the tool you need to debug if there is a chain of wrapped executables and it stops at the first.

A general point: from what I gathered in the past, the nested shebang problem only appears on Darwin (and some BSDs, but we don’t support them officially), not on Linux. Given there is a substantial decrease in debuggability with compiled wrappers, it would make sense to me to only use compiled wrappers when they are strictly necessary.

FRidh · 2020-09-09T11:35:39Z

I like this idea, why not exec nonetheless? Sounds like it would be hard to get the trace of exactly the tool you need to debug if there is a chain of wrapped executables and it stops at the first.

That's a good point.

A general point: from what I gathered in the past, the nested shebang problem only appears on Darwin (and some BSDs, but we don’t support them officially), not on Linux. Given there is a substantial decrease in debuggability with compiled wrappers, it would make sense to me to only use compiled wrappers when they are strictly necessary.

There are two issues:

darwin cannot execute nested shebangs
process name typically cannot be changed with interpreted programs. E.g., for Python we use a custom environment variable.

Now that I am writing this, I don't think it would actually solve 2.

Profpatsch · 2020-09-09T12:15:13Z

2\. process name typically cannot be changed with interpreted programs. E.g., for Python we use a custom environment variable.
Now that I am writing this, I don't think it would actually solve 2.

the process name is set via argv[0], right? And that is set in the execve(2) call coming from the caller.

2\. E.g., for Python we use a custom environment variable.

Do you have a reference to where you set that (and why)?

Profpatsch · 2020-09-09T12:16:58Z

Can you give some insight in why you chose nim over, like C/C++/rust/go for this rather base feature?

For the record, I think because of the nice split we can go forward with nim right now and rewrite it to a plain C program later if necessary, provided we keep the scope small.

Profpatsch · 2020-09-23T16:06:10Z

Do we have any explicit blockers on this PR? I’d love to see this merged.

FRidh · 2020-09-23T17:45:09Z

I've moved the code into a flake at https://github.com/FRidh/make-binary-wrapper. We can of course still package it here as well. Will need to update this PR then.

Do you have a reference to where you set that (and why)?

In a derivation using buildPythonPackage we set sys.argv[0] to the absolute path of the executable. We do this by injecting a piece of code. This is part of wrapPython.

In a python.buildEnv (and python.withPackages) derivation we need to wrap the existing executables. I thought we set sys.argv[0] in sitecustomize.py (pkgs/development/interpreters/python/sitecustomize.py) through an environment variable, but turns out I am wrong. We set only sys.executable and sys.prefix. This part really needs to be revised but that's another topic.

Profpatsch · 2020-10-08T17:02:32Z

We can of course still package it here as well. Will need to update this PR then.

I would love to merge this.

stale · 2021-04-07T02:53:52Z

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

roberth · 2021-09-30T14:21:35Z

#124556 seems to be a simpler solution with fewer dependencies, so it can be used more widely in Nixpkgs; including places where a minimal number of (bootstrapping) dependencies is desirable. For instance, it could feasibly be added to stdenv, whereas this could not, because it has Nim in the derivation closure.
Considering that this PR has been stale, wip, with open todos, and we have a more viable successor, I'll close it for now.

nixos-discourse · 2022-08-23T09:26:14Z

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

https://discourse.nixos.org/t/low-effort-and-high-impact-tasks/21095/41

FRidh commented Aug 16, 2020

View reviewed changes

FRidh mentioned this pull request Aug 16, 2020

[RFC] Declarative wrappers #85103

Closed

10 tasks

doronbehar requested changes Aug 16, 2020

View reviewed changes

ofborg bot added 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Aug 16, 2020

FRidh added the 2.status: work-in-progress This PR isn't done label Aug 16, 2020

FRidh mentioned this pull request Aug 16, 2020

Wrapper machinery should produce a tiny compiled binary #23018

Closed

FRidh added 9.needs: community feedback 6.topic: darwin Running or building packages on Darwin 9.needs: documentation labels Aug 16, 2020

FRidh added 2 commits August 16, 2020 17:44

nim: add minimal option that is stripped of its dependencies

da5f535

For `makeBinaryWrapper` we preferably keep the closure small so it can be used early on during bootstrapping.

FRidh force-pushed the make-binary-wrapper branch from 81e8e2b to da5f535 Compare August 16, 2020 15:44

FRidh requested a review from ehmry August 16, 2020 15:52

FRidh commented Aug 16, 2020

View reviewed changes

ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 labels Aug 16, 2020

ehmry reviewed Aug 22, 2020

View reviewed changes

ryantm added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Oct 3, 2020

FRidh mentioned this pull request Jan 15, 2021

[RFC 0075] Declarative wrappers NixOS/rfcs#75

Draft

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 7, 2021

FRidh mentioned this pull request Apr 27, 2021

makeWrapper doesn't cross-compile to windows #120726

Open

xaverdh mentioned this pull request May 28, 2021

Generate tiny compiled binary for wrapping executables #124556

Merged

9 tasks

ehmry added the 6.topic: nim Nim programing language label Sep 3, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Sep 3, 2021

roberth closed this Sep 30, 2021

FRidh mentioned this pull request May 6, 2022

wrapProgram should not move script files. instead, patch shebang lines #171689

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

makeBinaryWrapper: create binary wrappers #95569

makeBinaryWrapper: create binary wrappers #95569

FRidh commented Aug 16, 2020 •

edited

Loading

FRidh commented Aug 16, 2020 •

edited

Loading

FRidh Aug 16, 2020

FRidh Aug 16, 2020

doronbehar left a comment

doronbehar Aug 16, 2020

FRidh Aug 16, 2020 •

edited

Loading

doronbehar Aug 16, 2020

FRidh Aug 16, 2020

doronbehar commented Aug 16, 2020 •

edited

Loading

FRidh commented Aug 16, 2020

doronbehar commented Aug 16, 2020

FRidh Aug 16, 2020

ehmry Aug 22, 2020

andrew-d commented Aug 21, 2020

ehmry Aug 22, 2020

FRidh Aug 22, 2020

ehmry Aug 22, 2020

ehmry Aug 22, 2020

ehmry Aug 22, 2020 •

edited

Loading

FRidh Aug 22, 2020

ehmry Aug 22, 2020

Profpatsch commented Sep 9, 2020

FRidh commented Sep 9, 2020

ehmry commented Sep 9, 2020 •

edited

Loading

Profpatsch commented Sep 9, 2020 •

edited

Loading

FRidh commented Sep 9, 2020

Profpatsch commented Sep 9, 2020

Profpatsch commented Sep 9, 2020

Profpatsch commented Sep 23, 2020

FRidh commented Sep 23, 2020 •

edited

Loading

Profpatsch commented Oct 8, 2020

stale bot commented Apr 7, 2021

roberth commented Sep 30, 2021

nixos-discourse commented Aug 23, 2022


		# Wrapper type as used by the wrapper-generation code as well in the actual wrapper.

		type

makeBinaryWrapper: create binary wrappers #95569

makeBinaryWrapper: create binary wrappers #95569

Conversation

FRidh commented Aug 16, 2020 • edited Loading

Relevant issues

To do

Things done

FRidh commented Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doronbehar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh Aug 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

doronbehar commented Aug 16, 2020 • edited Loading

FRidh commented Aug 16, 2020

doronbehar commented Aug 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrew-d commented Aug 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ehmry Aug 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Profpatsch commented Sep 9, 2020

FRidh commented Sep 9, 2020

ehmry commented Sep 9, 2020 • edited Loading

Profpatsch commented Sep 9, 2020 • edited Loading

FRidh commented Sep 9, 2020

Profpatsch commented Sep 9, 2020

Profpatsch commented Sep 9, 2020

Profpatsch commented Sep 23, 2020

FRidh commented Sep 23, 2020 • edited Loading

Profpatsch commented Oct 8, 2020

stale bot commented Apr 7, 2021

roberth commented Sep 30, 2021

nixos-discourse commented Aug 23, 2022

FRidh commented Aug 16, 2020 •

edited

Loading

FRidh commented Aug 16, 2020 •

edited

Loading

FRidh Aug 16, 2020 •

edited

Loading

doronbehar commented Aug 16, 2020 •

edited

Loading

ehmry Aug 22, 2020 •

edited

Loading

ehmry commented Sep 9, 2020 •

edited

Loading

Profpatsch commented Sep 9, 2020 •

edited

Loading

FRidh commented Sep 23, 2020 •

edited

Loading