Skip to content

Conversation

mohsenari
Copy link
Collaborator

Summary

Added devbox install command. Re-used the same functions as devbox run since it's just an alias for calling devbox run -- : ( : is noop in shell).

How was it tested?

./devbox install

@hezhizhen
Copy link
Contributor

I think it's not necessary because when users run devbox shell, it ensures that all packages specified in devbox.json are installed (see internal/impl/devbox.go:L142).

@gcurtis
Copy link
Collaborator

gcurtis commented Mar 17, 2023

I like the concept, but I find the command name to be confusing. It's not immediately obvious what the difference is between devbox add and devbox install by their names. I'd also expect devbox install to install a package.

Some random ideas for names:

  1. Make devbox add (without a package) add missing packages that are in devbox.json.
  2. Make devbox run (without any args) a no-op. It starts a shell and immediately exits, triggering the package downloads.
  3. Have a devbox shell --prefetch or devbox shell --download flag.

@gcurtis
Copy link
Collaborator

gcurtis commented Mar 17, 2023

I think it's not necessary because when users run devbox shell, it ensures that all packages specified in devbox.json are installed (see internal/impl/devbox.go:L142).

@hezhizhen this is true, but we've had a few users ask for a way to trigger the package install manually for things like CI. You may want to know that your entire environment is downloaded and installed (and maybe cache it) before proceeding with other CI steps. Right now the workaround is to run something like devbox run -- exit, but it's not easy to figure that out.

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

My take:

  1. I agree we need this functionality
  2. I also agree with Greg that devbox install and devbox add are too similar.

I think devbox init should do an "install step" at the end. That way we avoid adding more top-level commands.

command := &cobra.Command{
Use: "install",
Short: "Installs all packages mentioned in devbox.json",
Long: "Starts a new shell and installs all packages mentioned in devbox.json or a json file" +
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 think this is accurate:

or a json file specified via --config

I believe we always look for a devbox.json file at the directory specified by --config. See these lines where we first get the "projectDir" and then append the hardcoded configFilename which is devbox.json:
https://github.com/jetpack-io/devbox/blob/bf52b9a6324f3d4fb65dc92ce1f67240f151cc9d/internal/impl/devbox.go#L88-L92

command := &cobra.Command{
Use: "install",
Short: "Installs all packages mentioned in devbox.json",
Long: "Starts a new shell and installs all packages mentioned in devbox.json or a json file" +
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we clarify that the shell is a devbox shell?
i.e. suggestion: "Starts a new devbox shell and ...."

Args: cobra.MaximumNArgs(0),
PreRunE: ensureNixInstalled,
RunE: func(cmd *cobra.Command, args []string) error {
err := runScriptCmd(cmd, []string{":"}, flags)
Copy link
Collaborator

Choose a reason for hiding this comment

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

for shell n00bs like myself, would you mind adding a small comment explaining that : is a noop in shell?

Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

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

Discussing IRL, the compelling argument is that init, install and add sub-commands are a common pattern for similar package managers like yarn, bundler, poetry.

Accepting modulo my suggestions below which I think are fairly straightforward to amend. Thanks!

@hezhizhen
Copy link
Contributor

Discussing IRL, the compelling argument is that init, install and add sub-commands are a common pattern for similar package managers like yarn, bundler, poetry.

Accepting modulo my suggestions below which I think are fairly straightforward to amend. Thanks!

I think add and install are too similar and it's better to have one alone (add package to devbox.json and install it). Users can manually edit devbox.json file (add or remove packages), and devbox provides searching functionality (like devbox search) to get valid package names and then users can directly edit devbox.json or through devbox install command.

@mohsenari
Copy link
Collaborator Author

I think add and install are too similar and it's better to have one alone (add package to devbox.json and install it).

@hezhizhen We had people asking for a way to install all their packages separately before going into devbox shell. Mostly, in CICD stages or with Dockerfile where installed packages can be cached as a layer in building stage of a container.
Most of the time we end up suggesting a hacky solutions such as devbox run -- echo "done" and to make this command set as an alias in their shell e.g., alias devbox-install="devbox run -- echo \"done\""
So this is to cover those cases. The add command still adds a package and installs it.

With all that said, do you have a nicer idea to implement instead of recommending devbox run -- echo "done" other than implementing devbox install?

devbox provides searching functionality (like devbox search)

This is a separate issue. We have been experimenting with different ideas and implementations of devbox search but we want to commit to an implementation that is sustainable for future development and scale.

@hezhizhen
Copy link
Contributor

We had people asking for a way to install all their packages separately before going into devbox shell. Mostly, in CICD stages or with Dockerfile where installed packages can be cached as a layer in building stage of a container.
Most of the time we end up suggesting a hacky solutions such as devbox run -- echo "done" and to make this command set as an alias in their shell e.g., alias devbox-install="devbox run -- echo "done""
So this is to cover those cases. The add command still adds a package and installs it.

You don't need to repeat what's been said before.

With all that said, do you have a nicer idea to implement instead of recommending devbox run -- echo "done" other than implementing devbox install?

My point is, should we have both of devbox add and devbox install, or we can combine them into one (like devbox install)?

This is a separate issue. We have been experimenting with different ideas and implementations of devbox search but we want to commit to an implementation that is sustainable for future development and scale.

Ignore it now.

@hezhizhen
Copy link
Contributor

Please ignore my comments, I'm not sure what's better.

@mohsenari
Copy link
Collaborator Author

@hezhizhen only having devbox install would imply removing devbox add which is difficult to do since most users would have to re-learn/re-implement devbox install [pkg] instead of devbox add [pkg].

Thanks a ton for providing your input on this. After a ton of discussion internally, we're gonna follow the pattern of well-known CLI tools and have a devbox install command. But it's only gonna be a small alias for devbox run -- : and not much more.

@mohsenari mohsenari merged commit c21f609 into main Mar 20, 2023
@mohsenari mohsenari deleted the mohsen--add-install-command branch March 20, 2023 19:12
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.

feature request: adding a devbox install command
5 participants