Skip to content

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Jan 5, 2023

Summary

This change allows devbox to work even if nix is not in path. This is useful for:

  • Right after installation we no longer require restarting terminal
  • After macOS updates (which clear /etc rc files) devbox should continue to work.

I tried a bunch of other approaches, but this one seems like most robust and least prone to errors because it does't actually change the commands. If we find a /nix dir but nix-shell is not in path, we attempt to source the nix startup files (multi and single user). If we still fail to find the binaries we show an error.

Edit:

Additional changes:

  • Added -y flag to avoid prompt
  • Moved some stuff out of nix package so that it doesn't depend on cobra commands/flags

cc: @LucilleH

How was it tested?

  • exported a PATH that was missing nix paths. Confirmed that nix version did not work. Devbox continued to work.
  • Fresh instal on ubuntu does not require restarting terminal

@mikeland73 mikeland73 requested review from gcurtis and loreto January 5, 2023 18:52
@mikeland73 mikeland73 force-pushed the landau/no-nix-in-path branch from 0d358aa to 7ada3b3 Compare January 6, 2023 20:56
@mikeland73
Copy link
Contributor Author

Tested fresh devbox/nix install on ubuntu and works without restarting terminal. @loreto @gcurtis PTAL


color.Yellow("\nNix is not installed. Devbox will attempt to install it.\n\n")

if skipPrompts, _ := cmd.Flags().GetBool(globalYesFlag); !skipPrompts {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we simply skip this if the terminal is non-interactive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do!

Comment on lines 30 to 32
"bash",
"-c",
fmt.Sprintf("source %s ; echo '<<<ENVIRONMENT>>>' ; env", srcFile),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this work with /bin/sh? bash isn't available in Alpine images, for example.

I'd also suggest clearing the environment to try and grab just the environment variables exported by the script (maybe even do a before-and-after diff). Otherwise we may end up reverting env vars that the user changed since starting their terminal.

Suggested change
"bash",
"-c",
fmt.Sprintf("source %s ; echo '<<<ENVIRONMENT>>>' ; env", srcFile),
"env",
"-i",
"/bin/sh",
"-c",
fmt.Sprintf(`. "%s" ; echo '<<<ENVIRONMENT>>>' ; env`, srcFile),

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gcurtis one issue your solution runs into is that the nix script itself uses __ETC_PROFILE_NIX_SOURCED to determine if it should run again. I can always pass that variable manually, but that's a bit fragile.

Question about variables changing, the commands Env should be exactly the same as the current process, from the docs:

// If Env is nil, the new process uses the current process's
// environment.

so the diff should in theory be just the loaded ones right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm switching bash to sh, but I don't think we can use a fresh environment. The reason is that the sourced files use environment variables (PATH and __ETC_PROFILE_NIX_SOURCED) this means that we would get bad values.

I actually think the risk of a stale variable is minimal because how this is used. It only gets called at pre-command time if /nix directory is found but nix is not in path. It never gets called again.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, good point. Alright, all sounds good to me!

@mikeland73 mikeland73 merged commit fe3fd45 into main Jan 9, 2023
@mikeland73 mikeland73 deleted the landau/no-nix-in-path branch January 9, 2023 19:31
@mikeland73 mikeland73 linked an issue Jan 13, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Nix installed to WSL2 doesn't source .profile
3 participants