Skip to content

Change welcome message to shell init hook #172

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Change welcome message to shell init hook #172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions devbox.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,15 @@ func (d *Devbox) Shell() error {
return errors.WithStack(err)
}
nixDir := filepath.Join(d.srcDir, ".devbox/gen/shell.nix")
sh, err := nix.DetectShell(nix.WithWelcomeMessage(plan.ShellWelcomeMessage))
sh, err := nix.DetectShell()
if err != nil {
// Fall back to using a plain Nix shell.
sh = &nix.Shell{}
}
sh.UserInitHook = d.cfg.Shell.InitHook.String()
initHook := ConfigShellCmds{
Cmds: plan.ShellInitHook,
}
sh.UserInitHook = initHook.String()
return sh.Run(nixDir)
}

Expand Down Expand Up @@ -188,6 +191,7 @@ func (d *Devbox) convertToPlan() *plansdk.Plan {
InstallStage: planStages[0],
BuildStage: planStages[1],
StartStage: planStages[2],
ShellInitHook: d.cfg.Shell.InitHook.Cmds,
}
}

Expand Down
3 changes: 1 addition & 2 deletions devbox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,7 @@ func assertPlansMatch(t *testing.T, expected *plansdk.Plan, actual *plansdk.Plan
"StartStage.InputFiles should match",
)

assert.ElementsMatch(expected.Definitions, actual.Definitions, "Definitions should match")
assert.Equal(expected.ShellWelcomeMessage, actual.ShellWelcomeMessage, "ShellWelcomeMessage should match")
assert.ElementsMatch(expected.PackageExtensions, actual.PackageExtensions, "PackageExtensions should match")
if expected.GeneratedFiles != nil {
assert.Equal(expected.GeneratedFiles, actual.GeneratedFiles, "GeneratedFiles should match")
}
Expand Down
11 changes: 1 addition & 10 deletions nix/shell.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,7 @@ type Shell struct {
userShellrcPath string

// UserInitHook contains commands that will run at shell startup.
UserInitHook string
welcomeMessage string
UserInitHook string
}

type ShellOption func(*Shell)
Expand Down Expand Up @@ -91,12 +90,6 @@ func DetectShell(opts ...ShellOption) (*Shell, error) {
return sh, nil
}

func WithWelcomeMessage(message string) ShellOption {
return func(s *Shell) {
s.welcomeMessage = message
}
}

// rcfilePath returns the absolute path for an rcfile, which is usually in the
// user's home directory. It doesn't guarantee that the file exists.
func rcfilePath(basename string) string {
Expand Down Expand Up @@ -249,12 +242,10 @@ func (s *Shell) writeDevboxShellrc() (path string, err error) {
OriginalInit string
OriginalInitPath string
UserHook string
WelcomeMessage string
}{
OriginalInit: string(bytes.TrimSpace(userShellrc)),
OriginalInitPath: filepath.Clean(s.userShellrcPath),
UserHook: strings.TrimSpace(s.UserInitHook),
WelcomeMessage: strings.TrimSpace(s.welcomeMessage),
})
if err != nil {
return "", fmt.Errorf("execute shellrc template: %v", err)
Expand Down
1 change: 0 additions & 1 deletion nix/shell_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,6 @@ func TestWriteDevboxShellrc(t *testing.T) {
s := &Shell{
userShellrcPath: test.shellrcPath,
UserInitHook: test.hook,
welcomeMessage: "Welcome to the devbox!",
}
gotPath, err := s.writeDevboxShellrc()
if err != nil {
Expand Down
10 changes: 0 additions & 10 deletions nix/shellrc.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,3 @@ export PS1="(devbox) $PS1"
# End Devbox User Hook

{{- end }}

# Begin Shell Welcome Message

{{- if .WelcomeMessage }}

echo "{{ .WelcomeMessage }}"

{{- end }}

# End Shell Welcome Message
6 changes: 0 additions & 6 deletions nix/testdata/shellrc/basic/shellrc.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,3 @@ export PS1="(devbox) $PS1"
echo "Hello from a devbox shell hook!"

# End Devbox User Hook

# Begin Shell Welcome Message

echo "Welcome to the devbox!"

# End Shell Welcome Message
6 changes: 0 additions & 6 deletions nix/testdata/shellrc/nohook/shellrc.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,3 @@ PATH="$(
export PS1="(devbox) $PS1"

# End Devbox Post-init Hook

# Begin Shell Welcome Message

echo "Welcome to the devbox!"

# End Shell Welcome Message
6 changes: 0 additions & 6 deletions nix/testdata/shellrc/noshellrc/shellrc.golden
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,3 @@ export PS1="(devbox) $PS1"
echo "Hello from a devbox shell hook!"

# End Devbox User Hook

# Begin Shell Welcome Message

echo "Welcome to the devbox!"

# End Shell Welcome Message
54 changes: 32 additions & 22 deletions planner/languages/nginx/nginx_planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,8 @@ func (p *Planner) IsRelevant(srcDir string) bool {

func (p *Planner) GetPlan(srcDir string) *plansdk.Plan {
return &plansdk.Plan{
ShellWelcomeMessage: fmt.Sprintf(welcomeMessage, p.shellConfig(srcDir)),
DevPackages: []string{
"nginx",
"shell-nginx",
},
RuntimePackages: []string{
"nginx",
Expand All @@ -45,11 +43,12 @@ func (p *Planner) GetPlan(srcDir string) *plansdk.Plan {
Command: fmt.Sprintf(startCommand, p.buildConfig(srcDir)),
InputFiles: plansdk.AllFiles(),
},
Definitions: []string{
fmt.Sprintf(nginxShellStartScript, srcDir, p.shellConfig(srcDir)),
},
GeneratedFiles: map[string]string{
"shell-helper-nginx.conf": fmt.Sprintf(shellHelperNginxConfig, os.TempDir()),
"shell-nginx.sh": fmt.Sprintf(nginxShellStartScript, srcDir, p.shellConfig(srcDir)),
},
ShellInitHook: []string{
fmt.Sprintf(". %s", filepath.Join(srcDir, ".devbox/gen/shell-nginx.sh")),
Comment on lines +48 to +51
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

},
}
}
Expand All @@ -68,18 +67,6 @@ func (p *Planner) buildConfig(srcDir string) string {
return "shell-nginx.conf"
}

const welcomeMessage = `
##### WARNING: nginx planner is experimental #####

You may need to add

\"include ./.devbox/gen/shell-helper-nginx.conf;\"

to your %s file to ensure the server can start in the nix shell.

Use \"shell-nginx\" to start the server
`

var startCommand = strings.TrimSpace(`
addgroup --system --gid 101 nginx && \
adduser --system --ingroup nginx --no-create-home --home /nonexistent --gecos "nginx user" --shell /bin/false --uid 101 nginx && \
Expand All @@ -92,12 +79,35 @@ var startCommand = strings.TrimSpace(`
`)

const nginxShellStartScript = `
shell-nginx = pkgs.writeShellScriptBin "shell-nginx" ''
#!/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")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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")

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
Comment on lines +99 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


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;"
Copy link
Collaborator

Choose a reason for hiding this comment

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

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;"

}
welcome
Comment on lines +82 to +109
Copy link
Contributor

Choose a reason for hiding this comment

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

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

`

const shellHelperNginxConfig = `access_log %[1]s/access.log;
client_body_temp_path %[1]s/client_body;
Expand Down
4 changes: 2 additions & 2 deletions planner/languages/php/php_planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NixLocalVariables would work 👍

}
if !plansdk.FileExists(filepath.Join(srcDir, "public/index.php")) {
return plan.WithError(usererr.New("Can't build. No public/index.php found."))
Expand Down Expand Up @@ -119,7 +119,7 @@ func (p *Planner) version(srcDir string) *plansdk.Version {
return latestVersion
}

func (p *Planner) definitions(srcDir string, v *plansdk.Version) []string {
func (p *Planner) pkgExtensions(srcDir string, v *plansdk.Version) []string {
extensions, err := p.extensions(srcDir)
if len(extensions) == 0 || err != nil {
return []string{}
Expand Down
36 changes: 20 additions & 16 deletions planner/plansdk/plansdk.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,18 +26,17 @@ type PlanError struct {

// Plan tells devbox how to start shells and build projects.
type Plan struct {
ShellWelcomeMessage string `json:"shell_welcome_message,omitempty"`

NixOverlays []string `cur:"[...string]" json:"nix_overlays,omitempty"`

// DevPackages is the slice of Nix packages that devbox makes available in
// its development environment. They are also available in shell.
DevPackages []string `cue:"[...string]" json:"dev_packages"`

// 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"`
Copy link
Collaborator Author

@LucilleH LucilleH Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@LucilleH LucilleH Sep 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

// InstallStage defines the actions that should be taken when
// installing language-specific libraries.
// Ex: pip install, yarn install, go get
Expand All @@ -50,15 +49,15 @@ type Plan struct {
// starting (running) the application.
// Ex: python main.py
StartStage *Stage `json:"start_stage,omitempty"`

Definitions []string `cue:"[...string]" json:"definitions"`
// Errors from plan generation. This usually means
// the user application may not be buildable.
Errors []PlanError `json:"errors,omitempty"`

// GeneratedFiles is a map of name => content for files that should be generated
// in the .devbox/gen directory. (Use string to make it marshalled version nicer.)
GeneratedFiles map[string]string `json:"generated_files,omitempty"`
// 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"`
Comment on lines +58 to +60
Copy link
Collaborator

Choose a reason for hiding this comment

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

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"`

}

type Planner interface {
Expand Down Expand Up @@ -118,10 +117,11 @@ func MergePlans(plans ...*Plan) (*Plan, error) {
err := mergo.Merge(
mergedPlan,
&Plan{
NixOverlays: p.NixOverlays,
DevPackages: p.DevPackages,
RuntimePackages: p.RuntimePackages,
Definitions: p.Definitions,
NixOverlays: p.NixOverlays,
DevPackages: p.DevPackages,
RuntimePackages: p.RuntimePackages,
PackageExtensions: p.PackageExtensions,
ShellInitHook: p.ShellInitHook,
},
// Only WithAppendSlice overlays, definitions, dev, and runtime packages fields.
mergo.WithAppendSlice,
Expand All @@ -135,7 +135,7 @@ func MergePlans(plans ...*Plan) (*Plan, error) {
plan.NixOverlays = pkgslice.Unique(mergedPlan.NixOverlays)
plan.DevPackages = pkgslice.Unique(mergedPlan.DevPackages)
plan.RuntimePackages = pkgslice.Unique(mergedPlan.RuntimePackages)
plan.Definitions = mergedPlan.Definitions
plan.ShellInitHook = mergedPlan.ShellInitHook

return plan, nil
}
Expand All @@ -144,7 +144,9 @@ func findBuildablePlan(plans ...*Plan) *Plan {
for _, p := range plans {
// For now, pick the first buildable plan.
if p.Buildable() {
return p
// Make a copy so that the original plan is not modified.
newP := *p
return &newP
Comment on lines +147 to +149
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
}
return &Plan{}
Expand All @@ -164,13 +166,15 @@ func MergeUserPlan(userPlan *Plan, automatedPlan *Plan) (*Plan, error) {
}

// Merging devPackages and runtimePackages fields.
packagesPlan, err := MergePlans(userPlan, automatedPlan)
// Order here matters because user defined shell hook should be executed last
mergedWithSlicePlan, err := MergePlans(automatedPlan, userPlan)
if err != nil {
return nil, errors.WithStack(err)
}

plan.DevPackages = packagesPlan.DevPackages
plan.RuntimePackages = packagesPlan.RuntimePackages
plan.DevPackages = mergedWithSlicePlan.DevPackages
plan.RuntimePackages = mergedWithSlicePlan.RuntimePackages
plan.ShellInitHook = mergedWithSlicePlan.ShellInitHook

return plan, nil
}
Expand Down
7 changes: 1 addition & 6 deletions testdata/nginx/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,9 @@
]
},
"dev_packages": [
"shell-nginx",
"nginx"
],
"runtime_packages": [
"nginx"
],
"definitions": [
"\nshell-nginx = pkgs.writeShellScriptBin \"shell-nginx\" ''\n\necho \"Starting nginx with command:\"\necho \"nginx -p testdata/nginx -c shell-nginx.conf -e /tmp/error.log -g \\\"pid /tmp/mynginx.pid;daemon off;\\\"\"\nnginx -p testdata/nginx -c shell-nginx.conf -e /tmp/error.log -g \"pid /tmp/shell-nginx.pid;daemon off;\"\n'';"
],
"shell_welcome_message": "\n##### WARNING: nginx planner is experimental #####\n\nYou may need to add\n\n\\\"include ./.devbox/gen/shell-helper-nginx.conf;\\\"\n\nto your shell-nginx.conf file to ensure the server can start in the nix shell.\n\nUse \\\"shell-nginx\\\" to start the server\n"
]
}
2 changes: 1 addition & 1 deletion testdata/php/php8.1/plan.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
"."
]
},
"definitions": [
"package_extensions": [
"php81 = pkgs.php81.withExtensions ({ enabled, all }: enabled ++ (with all; [ imagick mbstring ]));"
]
}
2 changes: 1 addition & 1 deletion tmpl/development.nix.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let
];
{{- end }}
};
{{- range .Definitions}}
{{- range .PackageExtensions}}
{{.}}
{{end -}}
in with pkgs;
Expand Down
2 changes: 1 addition & 1 deletion tmpl/runtime.nix.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let
];
{{- end }}
};
{{- range .Definitions}}
{{- range .PackageExtensions}}
{{.}}
{{end -}}
in with pkgs;
Expand Down
2 changes: 1 addition & 1 deletion tmpl/shell.nix.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ let
];
{{- end }}
};
{{- range .Definitions}}
{{- range .PackageExtensions}}
{{.}}
{{end -}}
in with pkgs;
Expand Down