tup: Let tup passthrough NIX_* env vars #60016

tfc · 2019-04-22T10:23:02Z

Motivation for this change

tup prunes all environment variables for all subprocesses that it starts. Some variables like PATH are passed through. This prevents nix-shell C and C++ environments from giving the compiler the right include and library folders for compiling/linking with/against external libraries.

At least the variables are needed for the typical c/c++ wrapper in nix:

NIX_CFLAGS_COMPILE
NIX_LDFLAGS
NIX_CC_WRAPPER_x86_64_unknown_linux_gnu_TARGET_HOST (If this is not set in the env, then the compiler wrapper will also not consume the other two vars, which is critical)

This PR adds a patch to tup which passes through all environment variables that are prefixed with NIX_ in order to provide a painless build experience in e.g. C++ projects with external libs.

This only affects the tup workflow in nix-shell. (For sandboxed nix build environments, users typically use tup generate to generate a shell script that performs the build process, because tup wants the SUID bit set, which is not allowed within nix-build)

Example:

Tup project with hello.cpp:

#include <boost/lexical_cast.hpp>
#include <iostream>

int main() {
  std::cout << "Hello World!\n"
    << "Boost: " << (BOOST_VERSION / 100000) << '.'
                 << (BOOST_VERSION / 100 % 1000) << '.'
                 << (BOOST_VERSION % 100) << '\n';
}

...and Tupfile:

: hello.cpp |> c++ hello.cpp -o hello |> hello

When building this with an unpatched tup in a nix-shell, you get the following error:

$ nix-shell -p boost tup --run "tup"
[ tup ] [0.000s] Scanning filesystem...
[ tup ] [0.000s] Reading in new environment variables...
[ tup ] [0.000s] No Tupfiles to parse.
[ tup ] [0.000s] No files to delete.
[ tup ] [0.000s] Executing Commands...
* 0) c++ hello.cpp -o hello                                                                                         
hello.cpp:1:10: fatal error: boost/lexical_cast.hpp: No such file or directory
 #include <boost/lexical_cast.hpp>
          ^~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
 *** tup messages ***
 *** Command ID=28 failed with return value 1
 [ ] 100%
 *** tup: 1 job failed.

The patched version correctly passes through all the nix vars to the compiler wrapper and then it works:

$ NIX_PATH=nixpkgs=patchednixpkgs nix-shell -p boost tup --run "tup"
[ tup ] [0.000s] Scanning filesystem...
[ tup ] [0.011s] Reading in new environment variables...
[ tup ] [0.011s] Parsing Tupfiles...
 0) [0.003s] .
 [ ] 100%
[ tup ] [0.020s] No files to delete.                                                                                              
[ tup ] [0.020s] Generating .gitignore files...
[ tup ] [0.026s] Executing Commands...
 0) [0.592s] c++ hello.cpp -o hello                                                                                               
 [ ] 100%
[ tup ] [0.628s] Updated.

...because this time the compiler got all the include folders right via NIX_CFLAGS_COMPILE.

(please note that in order to perform this at home one needs to delete the .tup directory in the project and rerun tup init because that folder's state would lead to the same old error)

An alternative to this patch is prepending the following lines to every tup project's root Tupfile:

export NIX_CFLAGS_COMPILE
export NIX_LDFLAGS
export NIX_CC_WRAPPER_x86_64_unknown_linux_gnu_TARGET_HOST

This does also work, but please note that:

This is the same for all tup projects just to be able to use nix-shell, so i'd personally prefer tup not taking these nix-specific env vars away in the first place
the NIX_CC_WRAPPER... env var name is arch- and OS-specific, so this is not a portable addition to tupfiles... The tup patch presented does it right, no matter what target platform is selected, because it is generic.

Things done

infinisil · 2019-04-22T22:38:54Z

I don't think this is a good idea. If people want a pure environment for nix-shell, they can use the --pure flag

tfc · 2019-04-23T09:30:35Z

@infinisil Thank you for your feedback! I extended the PR's description because i felt we were not talking about the same thing. I hope this change is now provided with enough context to make it acceptable.

infinisil

Ah I see, this looks good to me then, thanks for the explanation!

FRidh · 2019-04-27T07:45:09Z

Why not use tup generate in nix-shell? nix-shell is intended to replicate the behavior of nix-build, so one should also follow the same way of working.

tfc · 2019-04-27T07:49:10Z

@FRidh nix-shell is also intended to provide a developer workflow.

Please realize that scripts generated by tup generate buildscript.sh cannot build incrementally and they also do work completely sequential without parallelism.
This means that if you have a project that compiles for 20 minutes (which would compile only few minutes long on a multicore system) and change a single file, the script rebuilds the whole project for 20 minutes. This makes tup on nix completely unusable in developer workflows (without patching either tup or the tupfile in presented ways)

In order to check if nix-build will run correctly, you can still run tup generate buildscript.sh && ./buildscript.sh.
While using nix as a tool that installs you all the toolchain and dependencies for development, this patch allows you to use tup the normal way during development.

xzfc · 2019-04-29T03:54:10Z

Maybe this patch is good enough as a workaround, but I see potential issue with such wildcard-style variable inclusion.

Patched tup does not handle adding/removing/renaming of NIX_* variables.

Tupfile:

: |> env | grep '^NIX_ZZZ' > zzz.txt || true |> zzz.txt

Suppose I build with environment variable $NIX_ZZZ1 first, then rename it to $NIX_ZZZ2.

$ NIX_ZZZ1=a tup zzz.txt
<omitted>
$ cat zzz.txt
NIX_ZZZ1=a
$ NIX_ZZZ2=a tup zzz.txt
<omitted>
$ cat zzz.txt

Expected contents of zzz.txt is NIX_ZZZ2=a, but the file is empty.

You have to remove .tup (as mentioned in PR description). This goes against the tup philosophy ("Nor is there a need to do a "fresh checkout" and build your software from scratch" — quote from http://gittup.org/tup/manual.html). Also, you have to clean generated files manually or using external tools (tup lacks clean target by design), otherwise you'll get an error on subsequent build.

Note that this isn't an issue for $HOME variable since tup adds it to the database even if it is unset, as NULL value (see sqlite3 .tup/db '.mode insert' 'select * from var').

tfc · 2019-04-29T05:49:13Z

@xzfc thank you for your feedback.

Does it make the patch acceptable for you if it only passed through NIX_CFLAGS_COMPILE, NIX_LDFLAGS, and NIX_CC_WRAPPER_*_TARGET_HOST?

(with/without adding them to the DB like $HOME?)

infinisil · 2019-04-29T16:38:38Z

FYI, you can use NIX_CC_WRAPPER_${stdenv.cc.infixSalt}_TARGET_HOST for it to be platform-agnostic.

tfc · 2019-04-30T06:04:04Z

@infinisil thank you, but i was already aware of this and must say that it does not really make the nix-shell developer workflow with tup nice.

In the end one has to patch the Tupfiles with those 3 export clauses just to be able to build anything that has external dependencies. All workflows with tup and nix-shell i tried are frustrating, which is why i wrote this patch. (And it's also frustrating to only have non-parallel builds with tup build. This hinders acceptance of nix in certain projects)

xzfc · 2019-04-30T11:53:18Z

@tfc

Does it make the patch acceptable for you if it only passed through NIX_CFLAGS_COMPILE, NIX_LDFLAGS, and NIX_CC_WRAPPER_*_TARGET_HOST?

It still has a wildcard variable. And what about some other possible required variables (like NIX_HARDENING_ENABLE)?

Another idea: concatenate all required variables into a special variable and passthrough this variable.
Pack them with something like line below, and then unpack them back from within tup (maybe inside server_setenv()), or from compiler wrapper script.

export NIX_TUP_ENV="$(env -0 | grep -z '^NIX_' | grep -zv '^NIX_TUP_ENV=' | sed -z 's:\\:\\\\:g; s:\n:\\n:g' | tr '\0' '\n')"
# one variable per line, newlines and backslashes are escaped

This way tup will rebuild whenever a variable with prefix NIX_ is added, deleted, or updated.

Or maybe create an issue in the mainstream issue tracker?

tfc · 2019-05-04T12:55:27Z

@xzfc i see that my patch is really not enough (e.g. in cases with NIX_HARDENING_ENABLE etc.)

I am closing this PR and wil try to get something working that follows your idea with the NIX_TUP_ENV var. Thank you very much for your input!

Let tup passthrough NIX env vars

d7d252b

tfc changed the title ~~tup: Let tup passthrough NIX env vars~~ tup: Let tup passthrough NIX_* env vars Apr 22, 2019

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Apr 22, 2019

infinisil approved these changes Apr 23, 2019

View reviewed changes

tfc closed this May 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tup: Let tup passthrough NIX_* env vars #60016

tup: Let tup passthrough NIX_* env vars #60016

tfc commented Apr 22, 2019 •

edited

infinisil commented Apr 22, 2019

tfc commented Apr 23, 2019 •

edited

infinisil left a comment

FRidh commented Apr 27, 2019

tfc commented Apr 27, 2019 •

edited

xzfc commented Apr 29, 2019

tfc commented Apr 29, 2019 •

edited

infinisil commented Apr 29, 2019

tfc commented Apr 30, 2019

xzfc commented Apr 30, 2019

tfc commented May 4, 2019

tup: Let tup passthrough NIX_* env vars #60016

tup: Let tup passthrough NIX_* env vars #60016

Conversation

tfc commented Apr 22, 2019 • edited

Motivation for this change

Things done

infinisil commented Apr 22, 2019

tfc commented Apr 23, 2019 • edited

infinisil left a comment

Choose a reason for hiding this comment

FRidh commented Apr 27, 2019

tfc commented Apr 27, 2019 • edited

xzfc commented Apr 29, 2019

tfc commented Apr 29, 2019 • edited

infinisil commented Apr 29, 2019

tfc commented Apr 30, 2019

xzfc commented Apr 30, 2019

tfc commented May 4, 2019

tfc commented Apr 22, 2019 •

edited

tfc commented Apr 23, 2019 •

edited

tfc commented Apr 27, 2019 •

edited

tfc commented Apr 29, 2019 •

edited