[shell] add packages within shell, leveraging nix-profile #188

savil · 2022-09-29T20:39:34Z

Summary

Motivation:
We can define a custom nix-profile, and install packages scoped to this profile.

The goal is to make it easier to add packages when within devbox shell. Currently,
users need to exit the shell and restart it for the nix package to be installed - clearly
not ideal. With a nix-profile, if we install the package using that profile, then the
package's binary should be downloaded and visible within the shell.

Within devbox shell:

previously, we would manually add the path to each package to PATH.
now, we can add the path to the nix-profile to PATH and skip manually adding each package.

Limitations:

For devbox add within a devbox shell, the user needs to manually execute hash -r to ensure the latest binaries are visible.
~~For devbox rm within a devbox shell, the user still needs to restart the shell. I'm not quite sure why the nix-env --profile <profile dir> --uninstall <pkg> isn't working as intended.~~ Fixed via reapplying nix-env --profile <profile> -if development.nix
devbox add and rm when in a shell started from a parent directory will not work.
- A possible fix is to set DEVBOX_CONFIG_DIR env-var when inside a shell, and use that value in devbox add and rm.
- Example:

> cd testdata/rust/
> devbox shell rust-stable
(devbox)> devbox add go_1_18
Error: No devbox.json found in this directory, or any parent directories. Did you run `devbox init` yet?

How was it tested?

in testdata/rust/rust-stable:
- open devbox shell
- verify openssl not installed via which openssl and getting /usr/bin/openssl
- do devbox add openssl
  - note that which openssl still reflects the non nix-store location. It has NOT yet been updated.
- do hash -r and then which openssl and see it point to a nix-store location
- do devbox rm openssl
  - note that which openssl still reflects the nix-store location. It has NOT yet been updated.
- restart devbox shell and do which openssl to verify it has been removed

savil · 2022-09-29T20:39:47Z

Current dependencies on/for this PR:

main
- PR [config] look for the devbox.json file in parent directories as well #200
  - PR [shell] add packages within shell, leveraging nix-profile #188 👈
    - PR [shell] preserve history #204

This comment was auto-generated by Graphite.

LucilleH · 2022-10-03T23:39:10Z

boxcli/rm.go

+		fmt.Print(successMsg)
+
+		// Sadface. This doesn't seem to work within devbox shell for now.
+		fmt.Println(" You may need to restart `devbox shell` for this to take effect.")


I think that's reasonable.

Fixed this via doing nix-env --install development.nix again!

LucilleH · 2022-10-03T23:41:23Z

devbox.go

@@ -49,13 +49,26 @@ func Open(dir string) (*Devbox, error) {
 		return nil, errors.WithStack(err)
 	}

+	// if dir is current directory, then get the full path
+	if dir == "." {


solved via #200. Removed this code.

LucilleH · 2022-10-03T23:42:14Z

nix/shell.go

@@ -250,11 +258,13 @@ func (s *Shell) writeDevboxShellrc() (path string, err error) {
 		OriginalInitPath string
 		UserHook         string
 		PlanInitHook     string
+		NixProfileDir    string


nit: ProfileBinDir

LucilleH · 2022-10-03T23:42:59Z

nix/shellrc.tmpl

@@ -57,7 +61,7 @@ PATH="$(
 			*) non_nix_path="${non_nix_path:+$non_nix_path:}${path_dir}" ;;
 		esac
 	done
-	echo "${nix_path:+$nix_path:}${non_nix_path}"
+	echo "{{ .NixProfileDir }}:${non_nix_path}"


loreto · 2022-10-04T14:50:13Z

devbox.go

@@ -49,13 +49,26 @@ func Open(dir string) (*Devbox, error) {
 		return nil, errors.WithStack(err)
 	}

+	// if dir is current directory, then get the full path


Instead of only resolving ., why not call filpath.Abs on any directory, to ensure everything is an absolute path?

solved via #200

deleted this code

loreto · 2022-10-04T14:52:53Z

boxcli/add.go

@@ -27,6 +30,31 @@ func addCmdFunc() runFunc {
 		if err != nil {
 			return errors.WithStack(err)
 		}
-		return box.Add(args...)
+
+		if err = box.Add(args...); err != nil {


I think you have too much logic in here for add, rm and shell.

The goal of the devbox library is that it can be used as an SDK that is equivalent to using the CLI. In other words, box.Add("package") should be roughly the same as calling devbox add package; similarly box.Remove("package") should be roughly the same as calling devbox rm package. But since you've added all this logic inside of the CLI, instead of inside of the library, that is no longer true.

oh gotcha - sure I can move more logic inside the devbox library.

Do we want user-messaging to happen inside the library as well i.e. the print statements? Should we consider passing in an io.Writer in the constructor for box in devbox.Open?

loreto · 2022-10-04T14:58:08Z

nix/shell.go

@@ -107,7 +110,7 @@ func rcfilePath(basename string) string {
 	return filepath.Join(home, basename)
 }

-func (s *Shell) Run(nixPath string) error {
+func (s *Shell) Run(nixShellFilePath string, srcDir string) error {


I find this new interface for Run slightly confusing. Can you elaborate on why we now need both of these to be passed in?

I use it to construct the directory-for-profile-binaries i.e. srcDir + .devbox/profile/bin

It seems that the intention is for nix package to simulate a go-sdk. In that case, this is maybe not the best API.

loreto · 2022-10-04T14:59:54Z

devbox.go

 	box := &Devbox{
 		cfg:    cfg,
 		srcDir: dir,
 	}
 	return box, nil
 }

+func (d *Devbox) SourceDir() string {


I'm not sure this needs to be public – I think you have to make it public right now because you implemented a lot of the logic outside of the Devbox SDK – if you move that logic to be internal, then you don't need to expose this.

removed this function

savil · 2022-10-05T00:25:54Z

@loreto @LucilleH PTAL. Made a number of changes:

Pushed logic from CLI into devbox package.
Fixed shell.Exec() functionality
added nix.WithProfile api to handle the profileBinDir that is used in the shellrc file.

LucilleH

👍

LucilleH · 2022-10-05T01:06:54Z

devbox.go

+	if mode == uninstall {
+		installingVerb = "Uninstalling"
+	}
+	fmt.Fprintf(d.writer, "%s nix packages. This may take a while...", installingVerb)


Can we just say, "updating nix packages"?

LucilleH · 2022-10-05T01:21:21Z

devbox.go

+			installedVerb = "removed"
+		}
+
+		successMsg := fmt.Sprintf("%s is now %s.", pkgs[0], installedVerb)


Just say "packages updated?"

I had considered that, but "updated package" has another meaning i.e. has been updated from version 1.0.1 to 1.0.2 so felt it better to be super explicit

oh right, haven't thought about that hehe

LucilleH · 2022-10-05T01:31:12Z

devbox.go

+
+	cmdStr := fmt.Sprintf(
+		"--profile %s --install -f %s/.devbox/gen/development.nix",
+		d.srcDir+"/"+profileDir,


filepath.join?

oh yes of course :)

I'll audit other places in this PR too!

…stall development.nix' again

…fix shell.exec

savil force-pushed the savil/add-using-nix-profile branch 8 times, most recently from 8b711f3 to 513c9b4 Compare October 3, 2022 23:10

savil changed the title ~~[shell] use nix-profile~~ [shell] use nix-profile to enable add within shell Oct 3, 2022

savil changed the title ~~[shell] use nix-profile to enable add within shell~~ [shell] use nix-profile to enable adding packages within shell Oct 3, 2022

savil changed the title ~~[shell] use nix-profile to enable adding packages within shell~~ [shell] add packages within shell, leveraging nix-profile Oct 3, 2022

savil requested review from loreto and LucilleH October 3, 2022 23:14

savil marked this pull request as ready for review October 3, 2022 23:17

LucilleH approved these changes Oct 3, 2022

View reviewed changes

LucilleH mentioned this pull request Oct 4, 2022

Bug: "bash: command substitution" regression in 0.0.6 #196

Closed

loreto reviewed Oct 4, 2022

View reviewed changes

savil changed the base branch from main to savil/subfolder-search-config October 4, 2022 23:13

savil force-pushed the savil/add-using-nix-profile branch from 3fa3da9 to 7e8a62e Compare October 4, 2022 23:13

savil mentioned this pull request Oct 4, 2022

[config] look for the devbox.json file in parent directories as well #200

Merged

savil force-pushed the savil/subfolder-search-config branch from a4901dc to 8b8e7ab Compare October 4, 2022 23:16

savil force-pushed the savil/add-using-nix-profile branch 2 times, most recently from beffd38 to ac4992b Compare October 5, 2022 00:17

savil requested review from loreto and LucilleH October 5, 2022 00:24

savil force-pushed the savil/add-using-nix-profile branch from ac4992b to 8dfb76f Compare October 5, 2022 00:29

LucilleH reviewed Oct 5, 2022

View reviewed changes

savil mentioned this pull request Oct 5, 2022

[shell] preserve history #204

Merged

Base automatically changed from savil/subfolder-search-config to main October 5, 2022 16:18

savil force-pushed the savil/add-using-nix-profile branch from 8dfb76f to 40a2e33 Compare October 5, 2022 16:21

savil added 3 commits October 5, 2022 09:27

[shell] use nix-profile

8f0198c

fix remove inside shell, by calling 'nix-env --profile <profile> --in…

da112ea

…stall development.nix' again

Push CLI logic inside devbox package; add nix.WithProfile for shell; …

8305051

…fix shell.exec

savil force-pushed the savil/add-using-nix-profile branch from 40a2e33 to 8305051 Compare October 5, 2022 16:27

use filepath.Join

8bf8c99

savil added the feature New feature or request label Oct 5, 2022

savil merged commit b5d56f6 into main Oct 5, 2022

savil deleted the savil/add-using-nix-profile branch October 5, 2022 16:40

[shell] add packages within shell, leveraging nix-profile #188

[shell] add packages within shell, leveraging nix-profile #188

Uh oh!

Conversation

savil commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

savil commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

savil commented Oct 5, 2022

Uh oh!

LucilleH left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

savil commented Sep 29, 2022 •

edited

Loading

savil commented Sep 29, 2022 •

edited

Loading