Mostly replace profile scripts with Rust #738

grahamc · 2023-11-22T19:47:28Z

Try this PR:

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix/pr/738 | sh -s -- install

Description

(Includes clippy-nits, #737)

We rewrote the Nix installer in Rust to escape horrible and unpredictable bugs in the install scripts.
This has been super successful, but upstream still has a four critical scripts: the nix-profile* scripts.

Recently, we introduced a patch to add Nix's paths to XDG_DATA_DIRS: NixOS/nix#8985
This had an oversight, which was fixed in a PR: NixOS/nix#9312
...which also had an oversight which was released as 2.19.0, and fixed in a PR: NixOS/nix#9425

This quicksand style development nightmare is exactly why we rewrote it in the first place.
I examined the four versions (nix-profile{-daemon}.{fish,sh}) -- which all have different peculiarities, behaviors, and bugs.

Here are some of the issues I identified while trying to understand the differences.
Note that this isn't intended to be a "call-out" thread where I drag people through the mud or make Nix look bad or whatever.
I personally introduced some, and folks at DetSys introduced others.
This code was written by smart people, trying to do the right thing, but didn't have the tools to because scripting languages suck.

nix-profile-daemon.{fish,sh} protect against double-loading but nix-profile.{fish,sh} don't:

if [ -n "${__ETC_PROFILE_NIX_SOURCED:-}" ]; then return; fi
__ETC_PROFILE_NIX_SOURCED=1

if test -n "$__ETC_PROFILE_NIX_SOURCED"
exit
end
set __ETC_PROFILE_NIX_SOURCED 1

nix-profile-daemeon.fish sometimes leaks the add_path function, because it registers the function and then detects if Nix was sourced already:

function add_path --argument-names new_path
  # ...snip...
end

# Only execute this file once per shell.
if test -n "$__ETC_PROFILE_NIX_SOURCED"
  exit
end

# ...snip...

functions -e add_path

nix-profile.{fish,sh} check that HOME is defined before using it, but nix-profile-daemon.{fish,sh} don't, exposing users with set -u to a potential crash in the most common use case:
```
if [ -n "$HOME" ] && [ -n "$USER" ]; then
```
nix-profile.{fish,sh} requires that USER is defined, despite never using it. Likely leftover from a refactor around how GC roots were configured.
nix-profile{-daemon}.sh were updated to account for the XDG directory migration, but the Fish equivalent weren't:
```
NIX_LINK="$HOME/.nix-profile"
if [ -n "${XDG_STATE_HOME-}" ]; then
    NIX_LINK_NEW="$XDG_STATE_HOME/nix/profile"
else
    NIX_LINK_NEW="$HOME/.local/state/nix/profile"
fi
```
vs. the naive Fish:
```
set --export NIX_PROFILES "@localstatedir@/nix/profiles/default $HOME/.nix-profile"
```
By way of note, the XDG migration included a useful, user-forward migration path for users who had both a legacy and an XDG-based path. It was made defunct by a logical inversion by mistake,

nix-profile.sh, nix-profile.fish, nix-profile-daemon.sh all include the user's Nix profile in the XDG_DATA_DIRS, but nix-profile-daemon.fish does not:

set --export XDG_DATA_DIRS "$XDG_DATA_DIRS:$NIX_LINK/share:/nix/var/nix/profiles/default/share"

vs.

set --export XDG_DATA_DIRS "$XDG_DATA_DIRS:/nix/var/nix/profiles/default/share"

The profile scripts typically make an attempt at leaving the NIX_SSL_CERT_FILE environment variable alone if the user set it, but...
- nix-profile.sh doesn't bother
- nix-profile.{fish,sh} look to see if NIX_SSH_CERT_FILE (note the H!) is set instead of NIX_SSL_CERT_FILE (note the L).
All but nix-profile-daemon.sh will check to see if $NIX_LINK/etc/ca-bundle.crt exists and use that.
nix-profile-daemon.{sh,fish} both check for a file called etc/ssl/certs/ca-bundle.crt in all the defined NIX_PROFILES, but nix-profile.{sh,fish} don't.
nix-profile.{sh,fish} will extend MANPATH if it is already set, but the -daemon scripts won't.
The nix-profile-daemon.{fish,sh} scripts put /nix/var/nix/profiles/default/bin into the PATH, but the others don't. This is true, despite all four setting up the default profile.

So I experimented with rewriting these complicated and tricky bash profiles in Rust.

However, Rust can't set environment variables in the parent so we still need a bit of shell glue to finish the job.
The goal is to have as little shell as possible, in a way that ~never needs to be touched.

This is challenging, because we have to stick to the fundamentals of POSIX.
That means using read with a heredoc, or eval.

I don't like eval, but the heredoc method turns out to be very brittle. POSIX doesn't support \0, and this method implies trying to create a parser for the output in the most limited form of shell.

The aborted attempt at not using eval

The Rust program emits data like this:

ENVVARNAME ENVVARDATA
ENVVARNAME2 ENVVARDATA2

where ENVVARNAME is guaranteed to be a-zA-Z0-9_, and ENVVARDATA has no newlines or null bytes.
Then the profile script loads the environment variables like this:

# shellcheck shell=sh
if [ -f /nix/nix-installer ] && [ -x /nix/nix-installer ] && [ -z "${__ETC_PROFILE_NIX_SOURCED:-}" ]; then
    NIX_INSTALLER_EXPORT_DATA=$(/nix/nix-installer export --format space-newline-separated);
    while read -r nix_installer_export_key nix_installer_export_value; do
        export "$nix_installer_export_key=$nix_installer_export_value";
    done <<DATA_INPUT
$NIX_INSTALLER_EXPORT_DATA
DATA_INPUT

    unset NIX_INSTALLER_EXPORT_DATA nix_installer_export_key nix_installer_export_value
fi

...this is profoundly ugly, but it passes shellcheck and doesn't require touching for any logical changes to the environment handling.

Is it safe?

No.

Previous text before I started testing against a bunch of shells:

According to the POSIX spec and testing, yes.
IFS=' ' read -r key value is like a split_once, taking the first space-separated value of each line and storing it at $key, and then all remaining data of the line -- regardless of spaces -- is stored at $value.
This means any data with spaces, tabs, backslashes, etc. should be preserved.
The only rule is the data doesn't contain a newline -- which is protected against.

`eval`

Another way that can also be safe is to just use eval, since we know how to safely escape shell values:

eval "$(/nix/nix-installer export --format=sh)

The downside is this is much more "mystery-meat".
The nix-installer binary may be perceived as having more capabilities, though the capabilities aren't practically increased over the current implementation.
The user may also have concerns about the safety and correctness of the code we emit to be evaled, founded or unfounded.

Using eval is like a skepticism lightning-rod, however it is our safest and most resilient option.

Appendix of options that are not POSIX compliant

Null-byte separated output

This was the first try:

nix-installer export --format null-separated \
  | while IFS= read -r -d $'\0' key && IFS= read -r -d $'\0' value; do
    export "$key"="$value"
done

However, read -d and $'\0' aren't POSIX.

The space and newline separation, without the heredoc

nix-installer export --format space-newline-separated \
  | while IFS=' ' read key value; do
    export "$key"="$value"
done

Unfortunately this doesn't work either because of subshells.
The export "$key"="$value" step is executed in a subshell, which doesn't change the environment of the caller.
Bash 4.2+ does have shopt -s lastpipe, but again -- not POSIX.

Process substitution directly into the `while`

Like this:

while IFS=' ' read key value; do
  export "$key"="$value"
done < <(nix-installer export --format space-newline-separated)

However, < <(...) isn't POSIX.

Checklist

Formatted with cargo fmt
Built with nix build
Ran flake checks with nix flake check
Added or updated relevant tests (leave unchecked if not applicable)
Added or updated relevant documentation (leave unchecked if not applicable)
Linked to related issues (leave unchecked if not applicable)

TODO

Validate the fish parts on a real system
Add shellcheck to CI for the profile.sh and profile.fish scripts
Report upstream profile script bugs -- Assorted peculiarities of the various nix-profile{,-daemon}.{,fi}sh files NixOS/nix#9441
Double check the environment handling matches what I outlined in the list above
Especially double check MANPATH, which is probably wrong.

Open Questions

Is making our own profile.sh hook a liability we don't want to take? How can we do this most safely?

Validating with `install.determinate.systems`

If a maintainer has added the upload to s3 label to this PR, it will become available for installation via install.determinate.systems:

curl --proto '=https' --tlsv1.2 -sSf -L https://install.determinate.systems/nix/pr/$PR_NUMBER | sh -s -- install

src/cli/subcommand/export.rs

abathur · 2023-11-23T01:17:19Z

I can't find it at the moment, but I feel like I remember someone floating the idea of simplifying Nth shell support by replacing all of the scripts with a ~wrapper executable along the lines of env. Ensuring it's in place and used in the right places smells just as challenging, though?

src/cli/subcommand/export.rs

Hoverbear · 2023-11-28T17:43:42Z

Cargo.toml

@@ -61,6 +61,7 @@ which = "4.4.0"
 sysctl = "0.5.4"
 walkdir = "2.3.3"
 indexmap = { version = "2.0.2", features = ["serde"] }
+export = { git = "https://github.com/DeterminateSystems/export" }


Before this PR can merge this git dependency either needs to be published to crates.io or pulled into this repo as a workspace.

Since nix-installer is published to crates.io, we cannot have git deps.

grahamc · 2023-11-28T17:48:20Z

Definitely.

…

On Tue, Nov 28, 2023, at 12:43 PM, Ana Hobden wrote: ***@***.**** commented on this pull request. In Cargo.toml <#738 (comment)>: > @@ -61,6 +61,7 @@ which = "4.4.0" sysctl = "0.5.4" walkdir = "2.3.3" indexmap = { version = "2.0.2", features = ["serde"] } +export = { git = "https://github.com/DeterminateSystems/export" } Before this PR can merge this git dependency either needs to be published to crates.io or pulled into this repo as a workspace. Since `nix-installer` is published to crates.io, we cannot have git deps. — Reply to this email directly, view it on GitHub <#738 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAASXLEWZ6YYDSNSWMIHT53YGYPFTAVCNFSM6AAAAAA7WYBP52VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONJTGUYTINJTGU>. You are receiving this because you authored the thread.Message ID: ***@***.***>

Hoverbear · 2023-11-28T17:49:18Z

src/cli/subcommand/export.rs

+    #[error("__ETC_PROFILE_NIX_SOURCED is set, indicating the relevant environment variables have already been set.")]
+    AlreadyRun,
+
+    #[error("Some of the paths from Nix for XDG_DATA_DIR are not valid, due to an illegal character, like a colon.")]


We should print the paths, eg

Suggested change

#[error("Some of the paths from Nix for XDG_DATA_DIR are not valid, due to an illegal character, like a colon.")]

#[error("The paths {} from Nix for XDG_DATA_DIR are not valid, due to an illegal character, like a colon.", 0.iter().map(|v| format!("`{}`", v.display())).join(",")]

Hoverbear · 2023-11-28T17:56:42Z

src/cli/subcommand/export.rs

+/**
+Emit all the environment variables that should be set to use Nix.
+
+Safety note: environment variables and values can contain any bytes except


This is perhaps better suited to a normal comment below. This doc comment will print out in nix-installer export --help.

Hoverbear · 2023-11-28T17:59:39Z

src/cli/subcommand/export.rs

+for a null byte. This includes newlines and spaces, which requires careful
+handling.
+
+In `space-newline-separated` mode, `nix-installer` guarantees it will:


This and the following lines should be moved to a doc comment on the specific arg that reads well in the --help output.

Hoverbear · 2023-11-28T17:59:51Z

src/cli/subcommand/export.rs

+    format: ExportFormat,
+
+    #[clap(long)]
+    sample_output: Option<PathBuf>,


This argument needs documentation

Hoverbear · 2023-11-28T18:00:53Z

src/cli/subcommand/export.rs

+    async fn execute(self) -> eyre::Result<ExitCode> {
+        let env: HashMap<String, OsString> = match self.sample_output {
+            Some(filename) => {
+                // Note: not tokio File b/c I don't think serde_json has fancy async support?


Suggested change

// Note: not tokio File b/c I don't think serde_json has fancy async support?

// Note: not tokio::File because we are using serde_json::from_reader.

Hoverbear · 2023-11-28T18:03:29Z

src/cli/subcommand/export.rs

+            export_env.insert(k.try_into()?, v);
+        }
+
+        stdout().write_all(


While serde_json::from_reader may ask us to use std's sync readers, stdout and tokio do not require this. Could we use https://docs.rs/tokio/latest/tokio/io/fn.stdout.html ?

Hoverbear · 2023-11-28T18:05:25Z

src/cli/subcommand/export.rs

+        }
+
+        stdout().write_all(
+            export::escape(


A minor nit but I'd prefer if you let escaped = export::escape... and then used that in stdout().write_all(escaped.as_bytes()). This bit of code is kind cognitively complex. 😅

Hoverbear · 2023-11-28T18:18:55Z

src/cli/subcommand/export.rs

+    let mut envs: HashMap<String, OsString> = HashMap::new();
+
+    // Don't export variables twice.
+    // @PORT-NOTE nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented


Suggested change

// @PORT-NOTE nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented

// The nix install scripts' nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented

Hoverbear · 2023-11-28T18:19:10Z

src/cli/subcommand/export.rs

+        return Err(Error::AlreadyRun);
+    }
+
+    // @PORT-NOTE nix-profile.sh.in and nix-profile.fish.in check HOME and USER are set,


Suggested change

// @PORT-NOTE nix-profile.sh.in and nix-profile.fish.in check HOME and USER are set,

// The nix install scripts' nix-profile.sh.in and nix-profile.fish.in check HOME and USER are set,

Hoverbear · 2023-11-28T18:28:04Z

src/cli/subcommand/export.rs

+    {
+        let mut path = vec![
+            nix_link.join("bin"),
+            // Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.


Suggested change

// Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.

// Note: This is typically only used in single-user installs, doing it in both for simplicity.

Hoverbear · 2023-11-28T18:28:23Z

src/cli/subcommand/export.rs

+        let mut path = vec![
+            nix_link.join("bin"),
+            // Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.
+            // If there is good reason, we can make it fancier.


Suggested change

// If there is good reason, we can make it fancier.

Hoverbear · 2023-11-28T18:30:02Z

src/cli/subcommand/export.rs

+    if let Some(old_path) = env_path("MANPATH") {
+        let mut path = vec![
+            nix_link.join("share/man"),
+            // Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.


Suggested change

// Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.

// Note: This is typically only used in single-user installs, doing it in both for simplicity.

Hoverbear · 2023-11-28T18:30:08Z

src/cli/subcommand/export.rs

+        let mut path = vec![
+            nix_link.join("share/man"),
+            // Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.
+            // If there is good reason, we can make it fancier.


Suggested change

// If there is good reason, we can make it fancier.

Hoverbear · 2023-11-28T18:39:24Z

src/cli/subcommand/export.rs

+        }
+    }
+
+    if let Some(old_path) = env_path("MANPATH") {


Suggested change

if let Some(old_path) = env_path("MANPATH") {

if let Some(old_man_path) = env_path("MANPATH") {

Hoverbear · 2023-11-28T18:39:47Z

src/cli/subcommand/export.rs

+    }
+
+    if let Some(old_path) = env_path("MANPATH") {
+        let mut path = vec![


Suggested change

let mut path = vec![

let mut new_man_path = vec![

Hoverbear · 2023-11-28T18:40:03Z

src/cli/subcommand/export.rs

+            PathBuf::from(LOCAL_STATE_DIR).join("nix/profiles/default/share/man"),
+        ];
+
+        path.extend(old_path);


Suggested change

path.extend(old_path);

new_man_path.extend(old_man_path);

Hoverbear · 2023-11-28T18:40:23Z

src/cli/subcommand/export.rs

+    };
+
+    {
+        let mut path = vec![


Suggested change

let mut path = vec![

let mut new_path = vec![

Hoverbear · 2023-11-28T18:40:33Z

src/cli/subcommand/export.rs

+        ];
+
+        if let Some(old_path) = env_path("PATH") {
+            path.extend(old_path);


Suggested change

path.extend(old_path);

new_path.extend(old_path);

Hoverbear · 2023-11-28T18:40:41Z

src/cli/subcommand/export.rs

+            path.extend(old_path);
+        }
+
+        if let Ok(dirs) = env::join_paths(&path) {


Suggested change

if let Ok(dirs) = env::join_paths(&path) {

if let Ok(dirs) = env::join_paths(&new_path) {

Hoverbear · 2023-11-28T18:40:52Z

src/cli/subcommand/export.rs

+        if let Ok(dirs) = env::join_paths(&path) {
+            envs.insert("PATH".into(), dirs);
+        } else {
+            return Err(Error::InvalidPathDirs(path));


Suggested change

return Err(Error::InvalidPathDirs(path));

return Err(Error::InvalidPathDirs(new_path));

Hoverbear · 2023-11-28T18:44:49Z

src/cli/subcommand/export.rs

+    // Don't export variables twice.
+    // @PORT-NOTE nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented
+    // this behavior, but it was not implemented in nix-profile.sh.in and nix-profile.fish.in
+    // even though I believe it is desirable in both cases.


Let's not refer to ourselves as I in comments.

Suggested change

// even though I believe it is desirable in both cases.

// even though it is desirable in both cases.

Hoverbear · 2023-11-28T18:45:42Z

src/cli/subcommand/export.rs

+                        return Ok(ExitCode::SUCCESS);
+                    },
+                    Err(e) => {
+                        tracing::warn!("Error setting up the environment for Nix: {:?}", e);


Suggested change

tracing::warn!("Error setting up the environment for Nix: {:?}", e);

tracing::error!("Setting up the environment for Nix: {:?}", e);

Hoverbear · 2023-11-28T18:49:33Z

src/cli/subcommand/export.rs

+            },
+            None => {
+                match calculate_environment() {
+                    e @ Err(Error::AlreadyRun) => {


I'm not sure this is ideal. Returning errors invokes various backtrace handling which could add several microseconds to runtime.

Consider checking the env var on a new line between L82 and L83:

if nonempty_var_os("__ETC_PROFILE_NIX_SOURCED") == Some("1".into()) { tracing::debug!("Already ran export"); return Ok(ExitCode::SUCCESS); }

Then removing that error variant from the error type.

Hoverbear · 2023-11-28T18:49:49Z

src/cli/subcommand/export.rs

+const LOCAL_STATE_DIR: &str = "/nix/var";
+
+#[derive(Debug, thiserror::Error)]
+pub enum Error {


Let's try to avoid naming conflicts...

Suggested change

pub enum Error {

pub enum ExportError {

Hoverbear · 2023-11-28T19:23:31Z

I ran our VM test suite over this and it was without issues. Nice.

Hoverbear · 2023-11-29T18:28:19Z

Overall the code looks correct, only a few minor nits (above).

In this PR you describe several issues with the existing profile scripts: We should make sure each is reported (or fixed) upstream, even if we ultimately don't use them.

On a personal note, I don't love this. It makes me feel bad.

I don't love the installer running each time someone starts a new shell. (Okay fine, maybe we can consider it is 'installing' Nix into the shell.. Sure -- that's fine I guess.)

I don't love the installer doing something that seems more the domain of Nix itself.

I don't love having users look at the source script and seeing a call to a binary wrapped in an eval instead of a relatively plain bash file setting some environment variables.

I don't love that this will cause us to drift from upstream. I don't love that we'll now need to pay attention to changes in these files upstream and make sure we don't cause breakage for folks.

I also don't love that it's an several times (~8x in my testing) slower. I'm observing that bash -c '. /nix/nix-installer.d/profile.sh' takes approximately 8ms (7ms being our binary) using this branch. On the main branch of this installer, bash -c '. /nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh' takes ~1.2ms. (a tool with a similar job, starship, takes only 2.8ms)

I believe much of that time is related to starting the tokio executor, setting up logging, and doing arg parsing etc. I believe we can overcome this by using a different small executable, avoiding slow syscalls, or getting a bit clever with when we start up our tokio runtime, start our logging, and parse our clap args.

Regardless: We should address this slowdown if you want to shepherd this into a merge. 8ms on each shell start is an incredibly long time, even moreso on Mac where the remote builder fix causes it to be executed much more often then just on shell start.

I understand the motivations behind this change, but if I may be so bold: Many of the bugs originating from the shell scripts are not a problem with the scripts themselves, but the processes in which changes to those scripts are made. For example, NixOS/nix#8985 was not ready to merge, it was basically an invite for the Nix team to consider if they'd want such a thing. It should have had another pass from me, underwent more review, and have had a test added. None of those things happened. I planned to do those things with it but it was merged rather abruptly after it was decided it was the direction Nix wanted to.

This process problem is not something I have the ability to fix, I don't know if any of us do. There cannot be a culture of excellence around these scripts when, for example, I can very trivially go onto the Nix repository and find examples of contributors posting and merging their own PRs without review or discussion. That is not the kind of software engineering that is expected from foundational build technology that companies and individuals need to rely on, like Nix... that's cowboy coding. The kind of thing you do when you're a lone developer on a new prototype.

It is because of those process problems that I think we may want to opt for this solution: To help mitigate potential damage from future process related issues... But Nix is a product of those processes as well, so I don't really know.

Base automatically changed from clippy-nits to main November 22, 2023 19:50

grahamc added 4 commits November 22, 2023 14:51

Create a Rust helper for defining environment variables

a3231d4

clean up note

be02735

Don't turn an absent or empty path var into extra path values

848d223

fixup

f8dbb4d

grahamc force-pushed the replace-profile-scripts branch from 5685192 to f8dbb4d Compare November 22, 2023 19:51

grahamc added 3 commits November 22, 2023 15:22

spelling nit

e914be9

Fixup an issue where we incorrectly examine SSL cert locations

7907c62

Only extend MANPATH if it is set already

3b2713d

grahamc mentioned this pull request Nov 22, 2023

Assorted peculiarities of the various nix-profile{,-daemon}.{,fi}sh files NixOS/nix#9441

Open

Hoverbear reviewed Nov 22, 2023

View reviewed changes

src/cli/subcommand/export.rs Outdated Show resolved Hide resolved

grahamc added 2 commits November 22, 2023 15:55

fixup fish

7663d83

Don't try to set empty keys

f072444

grahamc mentioned this pull request Nov 22, 2023

Dump the environment in main's ci #739

Closed

6 tasks

Don't error if HOME isn't set, since it is apparentl ynot a problem?

dd8a3b9

IslandUsurper reviewed Nov 25, 2023

View reviewed changes

src/cli/subcommand/export.rs Outdated Show resolved Hide resolved

IslandUsurper reviewed Nov 25, 2023

View reviewed changes

src/cli/subcommand/export.rs Outdated Show resolved Hide resolved

IslandUsurper reviewed Nov 25, 2023

View reviewed changes

src/cli/subcommand/export.rs Outdated Show resolved Hide resolved

grahamc added 2 commits November 25, 2023 14:46

Update src/cli/subcommand/export.rs

b5a56d5

Clean up error handling

b5a4d15

grahamc force-pushed the replace-profile-scripts branch from b5e894b to b5a4d15 Compare November 25, 2023 19:57

grahamc added 4 commits November 25, 2023 14:59

Make calls to tracing logs explicit

3da6710

Filter out empty strings

d3f54df

Use eval instead ... I'll post an update about why later.

0e191ea

cargo lock

0ff7364

grahamc added the upload to s3 The labeled PR is allowed to upload its artifacts to S3 for easy testing label Nov 27, 2023

Fixup the profile paths in the post-install instructions

3063635

Hoverbear reviewed Nov 28, 2023

View reviewed changes

src/cli/subcommand/export.rs

};

{

let mut path = vec![

Copy link

Contributor

Hoverbear Nov 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change

let mut path = vec![

let mut new_path = vec![

Hoverbear reviewed Nov 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mostly replace profile scripts with Rust #738

Mostly replace profile scripts with Rust #738

grahamc commented Nov 22, 2023 •

edited

abathur commented Nov 23, 2023

Hoverbear Nov 28, 2023

grahamc commented Nov 28, 2023 via email

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023

Hoverbear Nov 28, 2023 •

edited

Hoverbear commented Nov 28, 2023

Hoverbear commented Nov 29, 2023

	#[error("Some of the paths from Nix for XDG_DATA_DIR are not valid, due to an illegal character, like a colon.")]
	#[error("The paths {} from Nix for XDG_DATA_DIR are not valid, due to an illegal character, like a colon.", 0.iter().map(\|v\| format!("`{}`", v.display())).join(",")]

	// Note: not tokio File b/c I don't think serde_json has fancy async support?
	// Note: not tokio::File because we are using serde_json::from_reader.

	// @PORT-NOTE nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented
	// The nix install scripts' nix-profile-daemon.sh.in and nix-profile-daemon.fish.in implemented

	// @PORT-NOTE nix-profile.sh.in and nix-profile.fish.in check HOME and USER are set,
	// The nix install scripts' nix-profile.sh.in and nix-profile.fish.in check HOME and USER are set,

	// Note: This is typically only used in single-user installs, but I chose to do it in both for simplicity.
	// Note: This is typically only used in single-user installs, doing it in both for simplicity.

	if let Some(old_path) = env_path("MANPATH") {
	if let Some(old_man_path) = env_path("MANPATH") {

	if let Ok(dirs) = env::join_paths(&path) {
	if let Ok(dirs) = env::join_paths(&new_path) {

	return Err(Error::InvalidPathDirs(path));
	return Err(Error::InvalidPathDirs(new_path));

	// even though I believe it is desirable in both cases.
	// even though it is desirable in both cases.

	tracing::warn!("Error setting up the environment for Nix: {:?}", e);
	tracing::error!("Setting up the environment for Nix: {:?}", e);

Mostly replace profile scripts with Rust #738

Are you sure you want to change the base?

Mostly replace profile scripts with Rust #738

Conversation

grahamc commented Nov 22, 2023 • edited

Description

The aborted attempt at not using eval

eval

Appendix of options that are not POSIX compliant

Null-byte separated output

The space and newline separation, without the heredoc

Process substitution directly into the while

Checklist

TODO

Open Questions

Validating with install.determinate.systems

abathur commented Nov 23, 2023

Choose a reason for hiding this comment

grahamc commented Nov 28, 2023 via email

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hoverbear Nov 28, 2023 • edited

Choose a reason for hiding this comment

Hoverbear commented Nov 28, 2023

Hoverbear commented Nov 29, 2023

grahamc commented Nov 22, 2023 •

edited

`eval`

Process substitution directly into the `while`

Validating with `install.determinate.systems`

Hoverbear Nov 28, 2023 •

edited