From 4332e8417d15f304741208e82ab969768f0dc11d Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 Aug 2023 16:16:44 -0400 Subject: [PATCH 1/3] drive-by fix to Python doc comment. --- src/bootstrap/bootstrap.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py index c5c70f2e18a11..c71ecc0a1396c 100644 --- a/src/bootstrap/bootstrap.py +++ b/src/bootstrap/bootstrap.py @@ -623,7 +623,7 @@ def _download_component_helper( def should_fix_bins_and_dylibs(self): """Whether or not `fix_bin_or_dylib` needs to be run; can only be True - on NixOS. + on NixOS or if config.toml has `build.patch-binaries-for-nix` set. """ if self._should_fix_bins_and_dylibs is not None: return self._should_fix_bins_and_dylibs From 3c6f4cc743316ff4b7f76a6492f78d6eb83dcf50 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 22 Aug 2023 17:45:54 -0400 Subject: [PATCH 2/3] Better diagnostics for people using nix subshell on non-NixOS. 1. Turned patch-binaries-for-nix from a boolean into a ternary flag: true, false, and unset. 2. When patch-binaries-for-nix is unset, we continue with the existing NixOS detection heuristic (look for nixos in /etc/os-release, if present), but if we are not atop NixOS, then issue a note if we see the IN_NIX_SHELL environment variable telling the user to consider setting patch-binaries-for-nix explicitly. --- src/bootstrap/bootstrap.py | 23 +++++++++++++++++++---- src/bootstrap/config.rs | 4 ++-- src/bootstrap/download.rs | 13 +++++++++++-- 3 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py index c71ecc0a1396c..588164e4a85c6 100644 --- a/src/bootstrap/bootstrap.py +++ b/src/bootstrap/bootstrap.py @@ -643,18 +643,33 @@ def get_answer(): if ostype != "Linux": return False - # If the user has asked binaries to be patched for Nix, then - # don't check for NixOS. + # If the user has explicitly indicated whether binaries should be + # patched for Nix, then don't check for NixOS. if self.get_toml("patch-binaries-for-nix", "build") == "true": return True + if self.get_toml("patch-binaries-for-nix", "build") == "false": + return False + + # Assume we should fix until we see evidence that it is not NixOS + should_fix_retval = True # Use `/etc/os-release` instead of `/etc/NIXOS`. # The latter one does not exist on NixOS when using tmpfs as root. try: with open("/etc/os-release", "r") as f: - return any(ln.strip() in ("ID=nixos", "ID='nixos'", 'ID="nixos"') for ln in f) + should_fix_retval = any(ln.strip() in ("ID=nixos", "ID='nixos'", 'ID="nixos"') for ln in f) except FileNotFoundError: - return False + should_fix_retval = False + + # If not on NixOS, then warn if user seems to be atop Nix shell + if not should_fix_retval: + in_nix_shell = os.getenv('IN_NIX_SHELL') + if in_nix_shell: + print("The IN_NIX_SHELL environment variable is set to `{}`;".format(in_nix_shell), + "you may need to set `patch-binaries-for-nix=true` in your config.toml", + file=sys.stderr) + + return should_fix_retval answer = self._should_fix_bins_and_dylibs = get_answer() if answer: diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 4821d20a89890..fc2c91f76b451 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -137,7 +137,7 @@ pub struct Config { pub json_output: bool, pub test_compare_mode: bool, pub color: Color, - pub patch_binaries_for_nix: bool, + pub patch_binaries_for_nix: Option, pub stage0_metadata: Stage0Metadata, pub stdout_is_tty: bool, @@ -1338,7 +1338,7 @@ impl Config { set(&mut config.local_rebuild, build.local_rebuild); set(&mut config.print_step_timings, build.print_step_timings); set(&mut config.print_step_rusage, build.print_step_rusage); - set(&mut config.patch_binaries_for_nix, build.patch_binaries_for_nix); + config.patch_binaries_for_nix = build.patch_binaries_for_nix; config.verbose = cmp::max(config.verbose, flags.verbose as usize); diff --git a/src/bootstrap/download.rs b/src/bootstrap/download.rs index 52162bf42ea42..bf8cb4bd76837 100644 --- a/src/bootstrap/download.rs +++ b/src/bootstrap/download.rs @@ -91,8 +91,8 @@ impl Config { // NOTE: this intentionally comes after the Linux check: // - patchelf only works with ELF files, so no need to run it on Mac or Windows // - On other Unix systems, there is no stable syscall interface, so Nix doesn't manage the global libc. - if self.patch_binaries_for_nix { - return true; + if let Some(explicit_value) = self.patch_binaries_for_nix { + return explicit_value; } // Use `/etc/os-release` instead of `/etc/NIXOS`. @@ -105,6 +105,15 @@ impl Config { matches!(l.trim(), "ID=nixos" | "ID='nixos'" | "ID=\"nixos\"") }), }; + if !is_nixos { + let in_nix_shell = env::var("IN_NIX_SHELL"); + if let Ok(in_nix_shell) = in_nix_shell { + eprintln!( + "The IN_NIX_SHELL environment variable is set to `{in_nix_shell}`; \ + you may need to set `patch-binaries-for-nix=true` in your config.toml" + ); + } + } is_nixos }); if val { From ec2c95e0932312c994ee8c5d52b7ee93b5ae65b9 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Wed, 23 Aug 2023 23:29:34 -0400 Subject: [PATCH 3/3] Accommodate tidy. In addition: Incorporated some review feedback (namely, removed a useless initial assignment to True that was never read), and unified code a bit more between bootstrap.py and download.rs (by using the same variable name for the same concept). --- src/bootstrap/bootstrap.py | 16 +++++++--------- src/bootstrap/download.rs | 4 ++-- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/bootstrap/bootstrap.py b/src/bootstrap/bootstrap.py index 588164e4a85c6..9dbc87c337c73 100644 --- a/src/bootstrap/bootstrap.py +++ b/src/bootstrap/bootstrap.py @@ -650,26 +650,24 @@ def get_answer(): if self.get_toml("patch-binaries-for-nix", "build") == "false": return False - # Assume we should fix until we see evidence that it is not NixOS - should_fix_retval = True - # Use `/etc/os-release` instead of `/etc/NIXOS`. # The latter one does not exist on NixOS when using tmpfs as root. try: with open("/etc/os-release", "r") as f: - should_fix_retval = any(ln.strip() in ("ID=nixos", "ID='nixos'", 'ID="nixos"') for ln in f) + is_nixos = any(ln.strip() in ("ID=nixos", "ID='nixos'", 'ID="nixos"') + for ln in f) except FileNotFoundError: - should_fix_retval = False + is_nixos = False # If not on NixOS, then warn if user seems to be atop Nix shell - if not should_fix_retval: + if not is_nixos: in_nix_shell = os.getenv('IN_NIX_SHELL') if in_nix_shell: - print("The IN_NIX_SHELL environment variable is set to `{}`;".format(in_nix_shell), - "you may need to set `patch-binaries-for-nix=true` in your config.toml", + print("The IN_NIX_SHELL environment variable is `{}`;".format(in_nix_shell), + "you may need to set `patch-binaries-for-nix=true` in config.toml", file=sys.stderr) - return should_fix_retval + return is_nixos answer = self._should_fix_bins_and_dylibs = get_answer() if answer: diff --git a/src/bootstrap/download.rs b/src/bootstrap/download.rs index bf8cb4bd76837..17c308e915b3f 100644 --- a/src/bootstrap/download.rs +++ b/src/bootstrap/download.rs @@ -109,8 +109,8 @@ impl Config { let in_nix_shell = env::var("IN_NIX_SHELL"); if let Ok(in_nix_shell) = in_nix_shell { eprintln!( - "The IN_NIX_SHELL environment variable is set to `{in_nix_shell}`; \ - you may need to set `patch-binaries-for-nix=true` in your config.toml" + "The IN_NIX_SHELL environment variable is `{in_nix_shell}`; \ + you may need to set `patch-binaries-for-nix=true` in config.toml" ); } }