Change welcome message to shell init hook #172

LucilleH · 2022-09-23T20:22:01Z

Summary

This PR changes the following:

Change welcome message to shell init hook
Change Definitions to packageExtensions

How was it tested?

go build
go test ./...
devbox shell

LucilleH · 2022-09-23T21:36:59Z

planner/plansdk/plansdk.go

 	// RuntimePackages is the slice of Nix packages that devbox makes available in
 	// in both the development environment and the final container that runs the
 	// application.
 	RuntimePackages []string `cue:"[...string]" json:"runtime_packages"`
+	// packageExtensions is the slice of Nix packages that devbox wants to extend
+	// to include extra packages that needs global installation.
+	PackageExtensions []string `cue:"[...string]" json:"package_extensions,omitempty"`


@mikeland86 for php, I wonder if we can use the same mechanism I did for nginx, in that we can create a folder .devbox/gen/php/ and add it to PATH, symlink the content it to the nix store, .devbox/gen/php/ folder is not immutable, thus they can install packages there instead? Then we can get rid of PackageExtensions entirely.

I think you may be misunderstanding what these do in php. They are not installing packages, they are recompiling PHP with the appropriate extensions. Some extensions are included by default but need to be "enabled". Other extensions are not included at all and require php to be built from source.

There's no way to just add an extension into a folder. The actual source of PHP needs to change.

Do we still need to recompile PHP if the store is mutable?

mikeland73 · 2022-09-23T21:55:51Z

FYI, this PR already replaces welcome messages with a planner shell hook #147

I had put the user shell hook and the planner shell hook separate, but I don't feel strongly on either one. Single shell hook may be simpler which is usually better.

LucilleH · 2022-09-23T22:25:20Z

@mikeland86 oh nice. Do you want to merge yours first and I can rebase? This PR also includes removing the need for definitions in nginx planner

mikeland73

Requesting changes because I think this PR needs more conversation.

I'm not sure entirely sure what some of the motivations behind this change are. Some comments:

I think merging welcome message into init hook makes sense. This PR #147 does that as well. I think merging the user and planner hook maybe makes sense, but need to think it through.
Putting scripts in .devbox instead of nix is ok. I guess it solves some mutability issues. This is a tradeoff.
I'm not entirely sure of the motivation of renaming Definitions to PackageExtensions. Feels like a nit. That section in the nix file does allow for pretty arbitrary definitions. Like I mentioned in my comment, I do not think we can get rid of this for PHP and possibly we will need it for other languages as well. For example right now the only way to get pandas to work with python is to put it in there (I was considering special casing a few python packages that don't currently work when installed via pip/poetry)

mikeland73 · 2022-09-23T22:10:27Z

planner/languages/nginx/nginx_planner.go

+			"shell-nginx.sh":          fmt.Sprintf(nginxShellStartScript, srcDir, p.shellConfig(srcDir)),
+		},
+		ShellInitHook: []string{
+			fmt.Sprintf(". %s", filepath.Join(srcDir, ".devbox/gen/shell-nginx.sh")),


I'm a bit unsure about splitting this out in this way. It feels a bit fragile. I almost want an option that does both (create a script and either calls it or sources it in the init hook)

Also, this should probably not be called shell-nginx.sh it it's an init script. Maybe just init.sh ?

mikeland73 · 2022-09-23T22:14:30Z

planner/plansdk/plansdk.go

 	// RuntimePackages is the slice of Nix packages that devbox makes available in
 	// in both the development environment and the final container that runs the
 	// application.
 	RuntimePackages []string `cue:"[...string]" json:"runtime_packages"`
+	// packageExtensions is the slice of Nix packages that devbox wants to extend
+	// to include extra packages that needs global installation.
+	PackageExtensions []string `cue:"[...string]" json:"package_extensions,omitempty"`


I think you may be misunderstanding what these do in php. They are not installing packages, they are recompiling PHP with the appropriate extensions. Some extensions are included by default but need to be "enabled". Other extensions are not included at all and require php to be built from source.

There's no way to just add an extension into a folder. The actual source of PHP needs to change.

LucilleH · 2022-09-23T22:33:45Z

I'm not entirely sure of the motivation of renaming Definitions to PackageExtensions. Feels like a nit. That section in the nix file does allow for pretty arbitrary definitions.

This is my issue with this. Now that we have ShellHook also in the plan, what goes where is a bit arbitrary. I moved the nginx stuff out of definitions because I think they don't necessarily need to be there. General environment variables and other scripts don't need to be there either.

Like I mentioned in my comment, I do not think we can get rid of this for PHP and possibly we will need it for other languages as well. For example right now the only way to get pandas to work with python is to put it in there (I was considering special casing a few python packages that don't currently work when installed via pip/poetry)

Yes and the only thing I can think of would be compiling the extension packages with the original like pip and python (which I think we can avoid that too). definitions is pretty vague to me, I have no idea what it means, and I don't want other shell scripts to end up there. If we ever want to replace the toolchain on top of the Nix store, the more general the harder. Thoughts? I'm open to renaming it to something different too. @mikeland86

mikeland73 · 2022-09-23T22:47:44Z

This is my issue with this. Now that we have ShellHook also in the plan, what goes where is a bit arbitrary. I moved the nginx stuff out of definitions because they do not belong there. General environment variables and other scripts don't need to be there either.

This is a tradeoff and, and we should just make sure we're aware we're making it. Putting scripts into nix store might give us more flexibility composing and merging planners in the future.

Yes and the only thing I can think of would be compiling the extension packages with the original like pip and python (which I think we can avoid that too). I'm open to renaming it to something different for now, but definitions is pretty vague to me, I have no idea what it means, and I don't want other shell scripts to end up there. If we ever want to replace the toolchain on top of the Nix store, the more general the harder. Thoughts? @mikeland86

Fair points. I think avoiding nix functions makes sense if we don't get benefit. It does feel a bit weird to "reinvent" nix functions. For example nix can create a shell script and put it in path. This PR simulates that with a shell hook. This does mean the nix files become less portable. Since they are already not super portable (because of shell hooks) that may not be the end of the world. Just a another tradeoff to keep in mind.

mikeland73

Merging plan hook and user hook seems fine to me, but we should rename the final hook to be more generic (e.g. ShellHook or ShellInitHook). Right now it is called UserHook which can be a bit misleading. We should also change the generated comments to indicate that as well.

mikeland73 · 2022-09-23T22:51:43Z

planner/languages/php/php_planner.go

@@ -52,7 +52,7 @@ func (p *Planner) GetPlan(srcDir string) *plansdk.Plan {
 			fmt.Sprintf("php%s", v.MajorMinorConcatenated()),
 			fmt.Sprintf("php%sPackages.composer", v.MajorMinorConcatenated()),
 		},
-		Definitions: p.definitions(srcDir, v),
+		PackageExtensions: p.pkgExtensions(srcDir, v),


Should we call these NixLocalVariables instead? extending packages is just one use case.

You could argue that we should avoid adding stuff here because it makes it harder to do nix free version, but would probably prefer a name that more closely resembles what it represents in the nix file.

NixLocalVariables would work 👍

mikeland73 · 2022-09-23T22:52:14Z

planner/languages/nginx/nginx_planner.go

+#!/bin/bash
+
+welcome() {
+	echo "##### WARNING: nginx planner is experimental #####

-echo "Starting nginx with command:"
-echo "nginx -p %[1]s -c %[2]s -e /tmp/error.log -g \"pid /tmp/mynginx.pid;daemon off;\""
-nginx -p %[1]s -c %[2]s -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"
-'';`
+	You may need to add
+
+	\"include shell-helper-nginx.conf;\"
+
+	to your %[2]s file to ensure the server can start in the nix shell.
+
+	Use \"shell-nginx\" to start the server"
+}
+
+pkg_path=$(readlink -f $(which nginx) | sed -r "s/\/bin\/nginx//g")
+conf_path=$pkg_path/conf
+
+mkdir -p %[1]s/.devbox/gen/nginx
+ln -sf $conf_path/* %[1]s/.devbox/gen/nginx/
+ln -sf $(pwd)/%[1]s/.devbox/gen/shell-helper-nginx.conf %[1]s/.devbox/gen/nginx/shell-helper-nginx.conf
+for file in %[1]s/*; do if [[ ! $file = .devbox ]]; then ln -sf $(pwd)/%[1]s/$file .devbox/gen/nginx/$file; fi; done
+
+shell-nginx() {
+	echo "Starting nginx with command:"
+	echo "nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g \"pid /tmp/mynginx.pid;daemon off;\""
+	nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"
+}
+welcome


This is long enough that is could benefit from being in own sh file and we can embed it.

gcurtis

This is probably just because of merge conflicts, but I think it would be nice to keep the user's shell commands and our commands separate with planInitHook. Otherwise LGTM.

gcurtis · 2022-09-26T19:22:30Z

planner/languages/nginx/nginx_planner.go

+	Use \"shell-nginx\" to start the server"
+}
+
+pkg_path=$(readlink -f $(which nginx) | sed -r "s/\/bin\/nginx//g")


Suggested change

pkg_path=$(readlink -f $(which nginx) | sed -r "s/\/bin\/nginx//g")

pkg_path=$(readlink -f "$(which nginx)" | sed -r "s/\/bin\/nginx//g")

gcurtis · 2022-09-26T19:26:45Z

planner/languages/nginx/nginx_planner.go

+mkdir -p %[1]s/.devbox/gen/nginx
+ln -sf $conf_path/* %[1]s/.devbox/gen/nginx/
+ln -sf $(pwd)/%[1]s/.devbox/gen/shell-helper-nginx.conf %[1]s/.devbox/gen/nginx/shell-helper-nginx.conf
+for file in %[1]s/*; do if [[ ! $file = .devbox ]]; then ln -sf $(pwd)/%[1]s/$file .devbox/gen/nginx/$file; fi; done


Suggested change

mkdir -p %[1]s/.devbox/gen/nginx

ln -sf $conf_path/* %[1]s/.devbox/gen/nginx/

ln -sf $(pwd)/%[1]s/.devbox/gen/shell-helper-nginx.conf %[1]s/.devbox/gen/nginx/shell-helper-nginx.conf

for file in %[1]s/*; do if [[ ! $file = .devbox ]]; then ln -sf $(pwd)/%[1]s/$file .devbox/gen/nginx/$file; fi; done

mkdir -p "%[1]s/.devbox/gen/nginx"

ln -sf "$conf_path/*" "%[1]s/.devbox/gen/nginx/"

ln -sf "$(pwd)/%[1]s/.devbox/gen/shell-helper-nginx.conf" "%[1]s/.devbox/gen/nginx/shell-helper-nginx.conf"

for file in "%[1]s"/*; do if [[ ! "$file" = .devbox ]]; then ln -sf "$(pwd)/%[1]s/$file" ".devbox/gen/nginx/$file"; fi; done

gcurtis · 2022-09-26T19:27:25Z

planner/languages/nginx/nginx_planner.go

+shell-nginx() {
+	echo "Starting nginx with command:"
+	echo "nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g \"pid /tmp/mynginx.pid;daemon off;\""
+	nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"


Suggested change

nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"

nginx -p "%[1]s" -c "%[1]s/.devbox/gen/nginx/%[2]s" -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"

gcurtis · 2022-09-26T19:33:52Z

planner/plansdk/plansdk.go

+	// This is an array of shell init hook that gets appended in front of
+	// the shell { initHook } defined by users.
+	ShellInitHook []string `cue:"[...string]" json:"shell_init_hook"`


Suggested change

// This is an array of shell init hook that gets appended in front of

// the shell { initHook } defined by users.

ShellInitHook []string `cue:"[...string]" json:"shell_init_hook"`

// ShellInitHook is a slice of shell commands to execute before the

// user's shell init hook. They only run in interactive devbox shells.

ShellInitHook []string `cue:"[...string]" json:"shell_init_hook"`

gcurtis · 2022-09-26T19:36:50Z

planner/plansdk/plansdk.go

+			// Make a copy so that the original plan is not modified.
+			newP := *p
+			return &newP


I don't know if this matters for this change, but ShellInitHook looks like it's the only field that will be copied. You'd need to do a deep copy if you want the pointers/slices/maps to be copied as well.

LucilleH · 2022-09-26T20:08:22Z

This is probably just because of merge conflicts, but I think it would be nice to keep the user's shell commands and our commands separate with planInitHook. Otherwise LGTM.

Yeah, I'm actually rethinking this - I might abandon this PR entirely, but, I feel like so much of the script is because of the immutability of the nix store. I wonder if I can leave the global nix store as is (immutable), but create a local mutable store per project (let's say .devbox/store) and symlink the bins to /nix/store. And the planners decide whether a mutable store is needed.

I might give that a try.

loreto · 2022-09-26T20:17:28Z

I wonder if I can leave the global nix store as is (immutable), but create a local mutable store per project (let's say .devbox/store) and symlink the bins to /nix/store. And the planners decide whether a mutable store is needed.

I might give that a try.

I believe that if we re-implement devbox add using nix profiles, that will happen automatically.

LucilleH · 2022-09-26T20:21:01Z

I believe that if we re-implement devbox add using nix profiles, that will happen automatically.

@loreto so does that mean each nix profile will be mapped to a devbox project? 🤔 Or each devbox.json picks the right nix profile?

loreto · 2022-09-26T20:24:24Z

I believe that if we re-implement devbox add using nix profiles, that will happen automatically.

@loreto so does that mean each nix profile will be mapped to a devbox project? 🤔 Or each devbox.json picks the right nix profile?

Each devbox project would have it's own nix profile, living in .devbox/profile

LucilleH · 2022-09-26T20:27:02Z

Each devbox project would have it's own nix profile, living in .devbox/profile

Oh perfect!!

LucilleH · 2022-09-26T20:29:06Z

closing this - .devbox/profile is a better way to solve it.

LucilleH force-pushed the lucille--init-hook branch 4 times, most recently from 7e43312 to f7527a3 Compare September 23, 2022 21:03

Change welcome message to shell init hook

e3546e2

LucilleH force-pushed the lucille--init-hook branch from f7527a3 to e3546e2 Compare September 23, 2022 21:28

LucilleH changed the title ~~[WIP] Change all hooks to shell init hook~~ [WIP] Change welcome message to shell init hook Sep 23, 2022

LucilleH changed the title ~~[WIP] Change welcome message to shell init hook~~ Change welcome message to shell init hook Sep 23, 2022

LucilleH marked this pull request as ready for review September 23, 2022 21:32

LucilleH requested review from mikeland73, savil and gcurtis September 23, 2022 21:32

LucilleH commented Sep 23, 2022

View reviewed changes

mikeland73 requested changes Sep 23, 2022

View reviewed changes

LucilleH requested a review from loreto September 23, 2022 22:42

mikeland73 approved these changes Sep 23, 2022

View reviewed changes

gcurtis approved these changes Sep 26, 2022

View reviewed changes

LucilleH marked this pull request as draft September 26, 2022 20:17

LucilleH closed this Sep 26, 2022

	pkg_path=$(readlink -f $(which nginx) \| sed -r "s/\/bin\/nginx//g")
	pkg_path=$(readlink -f "$(which nginx)" \| sed -r "s/\/bin\/nginx//g")

	nginx -p %[1]s -c %[1]s/.devbox/gen/nginx/%[2]s -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"
	nginx -p "%[1]s" -c "%[1]s/.devbox/gen/nginx/%[2]s" -e /tmp/error.log -g "pid /tmp/shell-nginx.pid;daemon off;"

Change welcome message to shell init hook #172

Change welcome message to shell init hook #172

Uh oh!

Conversation

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

Uh oh!

Summary

How was it tested?

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 commented Sep 23, 2022

Uh oh!

LucilleH commented Sep 23, 2022

Uh oh!

mikeland73 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!

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

Uh oh!

Uh oh!

mikeland73 commented Sep 23, 2022

Uh oh!

mikeland73 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!

gcurtis 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!

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

Uh oh!

Uh oh!

loreto commented Sep 26, 2022

Uh oh!

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

Uh oh!

Uh oh!

loreto commented Sep 26, 2022

Uh oh!

LucilleH commented Sep 26, 2022

Uh oh!

LucilleH commented Sep 26, 2022

Uh oh!

Uh oh!

LucilleH commented Sep 23, 2022 •

edited

Loading

LucilleH Sep 23, 2022 •

edited

Loading

LucilleH Sep 23, 2022 •

edited

Loading

LucilleH commented Sep 23, 2022 •

edited

Loading

LucilleH commented Sep 26, 2022 •

edited

Loading

LucilleH commented Sep 26, 2022 •

edited

Loading