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.

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

Jump to bottom

[script-on-error] disable for fish shell, and add testscript unit-test #1488

Merged
merged 3 commits into from
Sep 18, 2023
Merged

[script-on-error] disable for fish shell, and add testscript unit-test #1488

merged 3 commits into from
Sep 18, 2023

Conversation

savil
Copy link
Collaborator

@savil savil commented Sep 18, 2023 •
edited
Loading

Summary

set -e can fail in fish-shells (sometimes), so we disable script-on-error for them.

I also add a testscript unit-test for script-exit-on-error, in general.

How was it tested?

I run fish-shell, and did a sanity test that the go/hello-world example passes run_test.

BEFORE:
with this devbox.json

> cat devbox.json
{
    "packages": [
        "go@1.19.8"
    ],
    "shell": {
        "init_hook": [
            "export \"GOROOT=$(go env GOROOT)\"",
            "echo \"foo\""
        ],
        "scripts": {
            "run_test": "go run main.go"
        }
    }
}

doing devbox shell would give:

> devbox shell
Ensuring packages are installed.

Installing package: go@1.19.8.

[1/1] go@1.19.8
[1/1] go@1.19.8: Success
Starting a devbox shell...
set: -e: option requires an argument

~/code/jetpack/devbox/examples/development/go/hello-world/.devbox/gen/scripts/.hooks.sh (line 1):
set -e
^
from sourcing file ~/code/jetpack/devbox/examples/development/go/hello-world/.devbox/gen/scripts/.hooks.sh
	called on line 130 of file /var/folders/zv/r3sx92_94gq86_rq3yn1ky2h0000gn/T/devbox2997650637/config.fish
from sourcing file /var/folders/zv/r3sx92_94gq86_rq3yn1ky2h0000gn/T/devbox2997650637/config.fish

(Type 'help set' for related documentation)
foo
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish
(devbox)

AFTER:

> devbox shell
Ensuring packages are installed.

Installing package: go@1.19.8.

[1/1] go@1.19.8
[1/1] go@1.19.8: Success
Starting a devbox shell...
foo
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish
(devbox)>

@savil savil requested review from mikeland73, gcurtis and ipince and removed request for mikeland73 and gcurtis September 18, 2023 18:23
@savil Graphite App
Copy link
Collaborator Author

savil commented Sep 18, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

mikeland73
mikeland73 reviewed Sep 18, 2023
View reviewed changes
Copy link
Contributor

@mikeland73 mikeland73 left a comment

Choose a reason for hiding this comment

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

Wait, do we run scripts in the shell of choice? I thought we always used bash for portability. (Though looking at code it looks like we use sh or /bin/sh)

Fish in general is not a scripting language and more designed to be interactive only. (At least acording to Mr. Ridiculous Fish in this answer

Would this not break portability? At least with bash we know it will be installed. That's why plugins look for bash.

mikeland73
mikeland73 approved these changes Sep 18, 2023
View reviewed changes
internal/shellgen/scripts.go
@@ -25,6 +25,7 @@ type devboxer interface {
Lockfile() *lock.File
AllInstallablePackages() ([]*devpkg.Package, error)
InstallablePackages() []*devpkg.Package
IsUserShellFish() (bool, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not really introduced in this PR, but I think we should cut down on this interface pattern for pure data like this. (I admin to be much to blame for this)

There are some cases where the circular dependency is hard to avoid (for example if you need to call a function with side-effects that would create a cycle). If what you are passing in is pure data, cheap to compute (like in this case), then using a struct argument keeps code simpler and more linear.

Copy link
Collaborator Author

@savil savil Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I agree 💯

In this particular package, there are more public APIs than I think are good. For example, WriteScriptToFile should likely not be a public method. Having a tighter public interface, would enable a constructor which can take some of these arguments (as you suggest), minimizing the need for the interface.

@mikeland73
Copy link
Contributor

Approving, but added comments for discussion.

@savil
Copy link
Collaborator Author

savil commented Sep 18, 2023

@mikeland73 good point. The init-hooks will run in fish shell when doing devbox shell. Regular Devbox scripts will not run in fish shell, and will use sh (didn't verify, but IIRC you're correct).

I added a comment in code to clarify this, and also showed more output in the Test Plan.

@savil savil merged commit 3ed5546 into main Sep 18, 2023
@savil savil deleted the savil/fix-fish-script-on-error branch September 18, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeland73 mikeland73 mikeland73 approved these changes

@gcurtis gcurtis gcurtis approved these changes

@ipince ipince Awaiting requested review from ipince

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@savil @mikeland73 @gcurtis