godot: fix NIX_* environment propagations to scons

[trofi]

scons build system does not work by default in nixpkgs envoironment as it filters system environment and throws away NIX_* flags:

https://scons.org/doc/2.1.0/HTML/scons-user/x1750.html

Fix build system to always propagate os.environment.

Description of changes

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 23.05 Release Notes (or backporting 22.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[winterqt]

Hi! We were trying to decipher the previous version of this patch in #177578 (comment), but @Rotaerk brings up a good point: even without this patch, Godot builds fine -- why is that? Seemingly even without this patch, our environment variables are being propagated properly... did you run into a scenario where this doesn't happen? Thanks.

[trofi]

#210004 exposed variable filtering in many scons packages. godot was one of them: #210004 (comment)

The PR was reverted since and might not fail godot build anymore. But it still does variable filtering for some crucial $NIX_* variables. Or did upstream introduce do full ENV propagation to every command call?

[trofi]

Specifically, does NIX_CFLAGS_COMPILE_BEFORE get passed to cc if you remove the patch?

[Twey]

I don't believe that patch strips any environment variables that weren't already stripped by the SCons script. See this comment for an explanation of the patch. If anything, if SCons' Environment were now somehow responsible for adding good environment variables to PATH, PKG_CONFIG_PATH, or TERM, it would block that from happening — but it doesn't touch any other variables.

In current nixpkgs, I think Godot will probably build successfully without this patch in most environments — but the default behaviour here breaks hermeticity, so it will depend on the host system and the presence/absence of sandboxing. This is masked by the behaviour of cc-wrapper in environments with NIX_ENFORCE_PURITY set, which will quietly strip some impure paths from the link stage of the build — but will not fix any other impurities the use of global paths may introduce into the build up to that point.

[Twey]

On the other hand, I think that if you pass ENV = os.environ to Environment the rest of that patch is redundant, since that should already stop SCons from clobbering the OS environment. :) It looks like the only problematic value is PATH, which we're sure to override by the OS environment in a nixpkgs context.

[trofi]

The crucial detail of this patch is:

-env_base = Environment(tools=custom_tools)
+env_base = Environment(ENV = os.environ, tools=custom_tools)

(or ENV = os.environ). it's goal is not to strip any of the variables. Before this change it was stripping all the variables.

In current nixpkgs, I think Godot will probably build successfully without this patch in most environments

I'm not sure I agree. Doesn't godot already have to use other means to recover from excessive environment filtering? Like this one:

$ cat pkg_config_additions.patch
diff --git a/platform/x11/detect.py b/platform/x11/detect.py
index 98c9ddb..0cc2bff 100644
--- a/platform/x11/detect.py
+++ b/platform/x11/detect.py
@@ -255,6 +255,10 @@ def configure(env):
     env.ParseConfig("pkg-config xrender --cflags --libs")
     env.ParseConfig("pkg-config xi --cflags --libs")

+    env.ParseConfig("pkg-config xfixes --cflags --libs")
+    env.ParseConfig("pkg-config glu --cflags --libs")
+    env.ParseConfig("pkg-config zlib --cflags --libs")

Looks like an upstream configuration just does not work here.

but the default behaviour here breaks hermeticity, so it will depend on the host system and the presence/absence of sandboxing. This is masked by the behaviour of cc-wrapper in environments with NIX_ENFORCE_PURITY set, which will quietly strip some impure paths from the link stage of the build — but will not fix any other impurities the use of global paths may introduce into the build up to that point.

I am not sure if full environment filtering is the correct fix for hermeticity problems. It's not a SCons-specific problem. Or is it?

In my view the correct fix would be to fix the build system and package configure phase to prefer explicitly passed in lookup paths first and explicitly disable features that could leak in non-hermetic lookup paths. That way the non-hermetic paths even if present would not take practical part in the lookup.

On top of that environment filtering certainly drops $NIX_* variables that {cc,ld}-wrapper expect to receive. I personally find it very concerning.

If you still think this patch is wrong then feel free to drop it. godot will probably break again when I (or someone else) try another attempt at removing default include paths from nixpkgs's gcc.

[Twey]

No, I think your version of this patch is great. Mine hewed as closely to the original Godot behaviour as possible — you've gone much further and made sure it makes no changes to the nixpkgs-provided environment, which I think is exactly what we want to do. As you say, I think the original Godot behaviour was a bit of a hack to start with. I recommend we keep these changes.

[trofi]

On the other hand, I think that if you pass ENV = os.environ to Environment the rest of that patch is redundant

Yeah, that sounds about right.