Skip to content
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.

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

[shell] source the hooks file in shellrc #890

Merged
merged 5 commits into from
Apr 12, 2023
Merged

Conversation

savil
Copy link
Collaborator

@savil savil commented Apr 11, 2023

Summary

Also, fix up the rust example

How was it tested?

  • verify the hooks file is sourced in the generated shellrc

ran DEVBOX_DEBUG=1 devbox shell and inspected the output to find the temp-dir in which the shellrc was written to

...
144 # Source the hooks file, which contains the project's init hooks and plugin hooks.
145 . /Users/savil/code/jetpack/devbox/examples/development/rust/rust-stable-hello-world/.devbox/gen/scripts/.hooks.sh
...

Rust example

❯ devbox run -- 'echo $RUSTUP_HOME'
Ensuring packages are installed.
project dir is /Users/savil/code/jetpack/devbox/examples/development/rust/rust-stable-hello-world
info: using existing install for 'stable-x86_64-apple-darwin'
info: default toolchain set to 'stable-x86_64-apple-darwin'

  stable-x86_64-apple-darwin unchanged - rustc 1.68.2 (9eb3afe9e 2023-03-27)

/Users/savil/code/jetpack/devbox/examples/development/rust/rust-stable-hello-world/.rustup

and

> devbox shell

(devbox)
❯ echo "$RUSTUP_HOME"
/Users/savil/code/jetpack/devbox/examples/development/rust/rust-stable-hello-world/.rustup

Copy link
Collaborator Author

savil commented Apr 11, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@savil savil requested a review from ipince April 11, 2023 21:45
@savil
Copy link
Collaborator Author

savil commented Apr 11, 2023

🤔 I need to:

  1. update the fish shellrc file
  2. fix the shell_test that references UserInitHook and pluginInitHook (why did this pass locally?)

Copy link
Contributor

@ipince ipince left a comment

Choose a reason for hiding this comment

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

LGTM but see comments

@@ -1 +1 @@
echo "Hello from a devbox shell hook!"
/path/to/hooksfile/hooks.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@@ -1 +1 @@
echo "Hello from a devbox shell hook!"
/path/to/hooksfile/hooks.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this file this? i dont get it

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it contains a (fake) path-to-the-hooks file.

I could: rename the file from hooks.sh to path-to-hooks-file.txt.
Not sure how else to capture the two scenarios of this hooks file being missing or not. 🤔 is it always present?

Copy link
Collaborator Author

@savil savil Apr 12, 2023

Choose a reason for hiding this comment

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

ah, okay, we always write this. Confirmed by doing devbox init for an empty project, DEVBOX_DEBUG=1 devbox shell and inspecting the generated shellrc file: We source a hooks file which exists but is empty.

I will:
(a) delete this shellrc/nohook test case
(b) expose a function for the hooksFilePath and refer to that in the testcase logic.

if err != nil {
return err
}

env, err := d.nixEnv(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

somewhere around here, we should ensure the script files are written. see the run impl

you can test if they're being written by deleting .devbox/gen/scripts and then running devbox shell

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

on line 164: d.ensurePackagesAreInstalled
-> d.generateShellFiles
-> d.writeScriptsToFiles

@savil savil force-pushed the savil/source-hooks-in-shellrc branch from 9352874 to fa9dbb7 Compare April 12, 2023 17:22
@savil
Copy link
Collaborator Author

savil commented Apr 12, 2023

@ipince please see last commit. I made a few changes.

Comment on lines +839 to +847
written[d.scriptPath(hooksFilename)] = struct{}{}

// Write scripts to files.
for name, body := range d.cfg.Shell.Scripts {
err = d.writeScriptFile(name, d.scriptBody(body.String()))
if err != nil {
return errors.WithStack(err)
}
written[d.scriptFilename(name)] = struct{}{}
written[d.scriptPath(name)] = struct{}{}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ipince these two changes have a small logic change: the map's key was filename and is not filepath. I think this is safe.

Changes:
- refactor: use d.scriptPath everywhere, and delete d.scriptFilename
- remove nohook shell test case
- remove `hook` files from shell test cases
@savil savil force-pushed the savil/source-hooks-in-shellrc branch from fa9dbb7 to 6918c39 Compare April 12, 2023 17:39
@savil savil merged commit ea6fb6d into main Apr 12, 2023
9 checks passed
@savil savil deleted the savil/source-hooks-in-shellrc branch April 12, 2023 17:59
mikeland73 added a commit that referenced this pull request Apr 19, 2023
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.

None yet

2 participants