The settings on the temporary variables should be fully addressed with #97597.

That isn't to say though that not setting NIX_ENFORCE_PURITY is or isn't a good idea. That will depend on whether someone is more likely to having impure paths sneak in or have pure paths not resolve correctly.

In the past one person ran into issues due to making their home directory a symlink. This resulted in their being multiple paths to the same libraries, only one of which was recognized as being pure. Not settings NIX_ENFORCE_PURITY would have avoided this. Possible also if the wrapper script had set NIX_BUILD_TOP to $(realpath $HOME/.theano) instead of just $HOME/.theano things would also have been okay.

All I did though was advice them not to use symlinks for their home as it is generally a bad idea in my experience. For example, if /home/me is a symlink to /mnt/external/home, then /home/me/.. is /mnt/external and not /home, which I've seen cause issues before.

It is likely also worth mentioning, now that I'm more familiar with cross compilation, technically we should have the runtime version of the buildtime compiler. This hasn't been an issue yet though as I don't think anytime is cross compiling theano. 😁

I'm proposing this PR because it isn't fixed. Theano on master will build fine, but as soon as you try to import it, it will give the same error. Do try it.

In particular this means that import theano will fail when done from inside a sandbox, so the pythonImportsCheck added here would otherwise fail and it also causes the tests of downstream packages such as pymc3 to fail.

That is because #97597 was just recently merged into staging, so it hasn't yet landed in master

https://github.com/NixOS/nixpkgs/compare/staging

Ahh staging - I didn't notice that. 👍

I'm looking at something that will be safely backportable to 20.09 though, and I'm not sure how people will feel about a significant compiler-wrapper change.

I'm not opposed to this patch. I just don't know if it will be a net win or not.

From a purity standpoint, I prefer the purity filtering. From a practical standpoint though, it if we keep running into corner cases, it is better to forgo it. What we don't know is if we are going to discover corner cases from disabling it because the purity enforcement isn't just about checking, it also actively strips possibly problematic paths from the users environment (IIRC, this was an issue on our CentOS based super computer clusters).

If we do merge this, my suggestion would be to pass through the user's NIX_ENFORCE_PURITY rather than unsetting it

"$(declare -xp | sed -e '/^[^=]\+="\('"''${NIX_STORE//\//\\/}"'\|[^\/]\)/!d' \
                     -e '/declare -x \(NIX_ENFORCE_PURITY\)/d'))"

so, if there are people who require it, or we think it might help them, there will be a way to re-enable it by manual exporting NIX_ENFORCE_PURITY=1 before running Theano.

Another, possibly better, alternative would be to combine this with clearing the user environment before invoking the compiler

#!$(type -P env) -i $(type -P bash)

as I expect this may also be sufficient to resolve the bad user's environment issues that the purity checking took care of.

Assuming this last one works, I would suggest merging that into 20.09. It would probably be fine for master too, if people really wanted it, although my slight personal preference would be to find out first where things sit with hydra once staging lands.

Ok let's wait for staging to land. Though I'm sceptical that even if everything is happy, the staging solution will be appropriate for 20.09 given how close we're getting. Patching things here can only ever break theano-related packages, which is why I like it.

I'm also running the implications of fully stripping env for the compiler wrapper through my mind...

Well, the compiler environment wouldn't be fully stripped. It would be the build environment variables stripped of any variables set to absolute paths other than those under $NIX_STORE. That is, the command

declare -xp

prints all the bash declare command required to restore the exported environment. The sed line then removes exported environment variables from that that aren't of the form

• <absolute path starting with $NIX_STORE> or
• <not an absolute path>

which is what gets put at the start of the gcc wrapper script.

Still, it is a bit of an approximation though. From the gcc environment variables page, you can see there are both environment variables you wouldn't want leaking to the compiler (e.g., LIBRARY_PATH, CPATH, C_INCLUDE_PATH, etc.) and some that should (e.g., TMPDIR). And that is just gcc. You have to also consider the compiler could be clang.

This is what makes hijacking the existing nix purity filtering system seem so tempting.

I think the problem with respecting the user's NIX_ENFORCE_PURITY setting in the wrapper is it will cause failures when invoked from within another package's build, because of course, NIX_ENFORCE_PURITY will be set. So it will e.g. cause downstream packages' tests to fail.

NIX_ENFORCE_PURITY now shouldn't be causing problems on master or 20.09, right?

It's still causing problems on 20.09 because it lacks #97597 (and its predecessor), and I think it's probably too significant a change to introduce to 20.09 at this stage.

However I have figured out a workaround I can use for 20.09, which I'm about to open a PR for shortly. (edit: this is #100955)

Certainly this PR can be closed.