[planners] Separate shell plan from build plan #227

LucilleH · 2022-10-10T18:30:58Z

Summary

This PR separate shell plans from build plans. This PR looks big but really it only does the following:
For shell

Separate planner struct into shellPlan and buildPlan
Shell plan is applied when user defined packages ~~include all of the packages the shell plan requires~~ include one of the packages from the shell plan.
If multiple shell plans are found and applicable, the fields are merged and appended
If no shell plans are applicable, launch shell with user defined devbox.json
If we detect that the user defined packages do not include all of shell plan suggested packages, we print a warning and suggest user to run devbox add

For build

Build plan is only applied when user defined packages includes all of the dev packages the build plan requires
Build plan runtimePackages are no longer merged with user defined packages
If multiple buildable plans are found and applicable, only 1 build plan is selected
If no buildable plans are found, return error.

How was it tested?

go test ./...
devbox shell
devbox build

Until we can land #227, I want to revert the `devbox.go` changes in #222 so that shell plans can continue working, and we can release some other (unrelated) fixes. In this PR, I revert (manually) the `devbox.go` changes in #222, while leaving the test changes to minimize merge conflicts with #227.

@loreto

…228) ## Summary Until we can land #227, I want to revert the changes in #222 so that shell plans can continue working, and we can release some other (unrelated) fixes. In this PR, I revert (manually) the `devbox.go` changes in #222, while leaving the test changes to minimize merge conflicts with #227. cc @loreto @LucilleH ## How was it tested? sanity check ``` > cd testdata/nodejs/nodejs-18 > devbox shell > which node # get nix store path > exit ``` - [x] will find an example with init-hooks and test that. Example 1: nginx ``` > devbox shell Installing nix packages. This may take a while...done. Starting a devbox shell... ##### WARNING: nginx planner is experimental ##### You may need to add "include ./.devbox/gen/shell-helper-nginx.conf;" to your shell-nginx.conf file to ensure the server can start in the nix shell. Use "shell-nginx" to start the server ``` Example 2: python/pip-example ``` devbox/testdata/python/pip-example > devbox shell Installing nix packages. This may take a while...done. Starting a devbox shell... Creating/Using virtual environment in /Users/savil/code/jetpack/devbox/testdata/python/pip-example/.venv (devbox) devbox/testdata/python/pip-example > which python3 /Users/savil/code/jetpack/devbox/testdata/python/pip-example/.venv/bin/python3 ```

savil

Nice!

Overall, I really like the separation of ShellPlan from BuildPlan. I think this will add clarity.

The parts that have me confused are:

Shell plan is only applied when user defined packages includes all of the packages the shell plan requires
Build plan is only applied when user defined packages includes all of the dev packages the build plan requires
Build plan runtimePackages are no longer merged with user defined packages

What is the overall story around how users will understand this behavior?
It feels a bit of a stretch still that users are expected to first add the package via devbox add for devbox's planners to kick in. cc @Lagoja @loreto curious about your thoughts.

savil · 2022-10-12T00:18:55Z

boxcli/plan.go


-	plan, err := box.BuildPlan()
+	shellPlan := box.ShellPlan()


check shellPlan.Invalid() ?

There's no invalid shell plan, as devbox shell should always succeeds?

savil · 2022-10-12T00:26:51Z

devbox.go

-	return d.convertToPlan()
+func (d *Devbox) ShellPlan() *plansdk.ShellPlan {
+	shellPlan := planner.GetShellPlan(d.srcDir, d.cfg.Packages)
+	shellPlan.DevPackages = d.cfg.Packages


Two thoughts when reading this. It's possible the answers await me below as I read on.

Should this override or append to DevPackages?

Also, which not do this inside planner.GetShellPlan?

Since shell plans are only selected when users have all of the packages a shell plan expect, user defined packages will always include devPackages from shell plan.

I was thinking planner.GetShellPlan only strictly return the shell plan from planners.

savil · 2022-10-12T00:30:51Z

devbox.go

 	if err != nil {
 		return nil, err
 	}
-	return plansdk.MergeUserPlan(userPlan, buildPlan)
+	return plansdk.MergeUserBuildPlan(userPlan, buildPlan)


same thought as above. Check for buildPlan.Invalid() here?

Right now we have Buildable() and Invalid(). I think maybe we can consolidate the 2. I'll refactor a bit 👍

savil · 2022-10-12T00:34:30Z

devbox_test.go


 	currentDir, err := os.Getwd()
 	require.New(t).NoError(err)

 	baseDir := filepath.Dir(testPath)
 	t.Run(baseDir, func(t *testing.T) {
 		assert := assert.New(t)
-		goldenFile := filepath.Join(baseDir, "plan.json")
-		hasGoldenFile := fileExists(goldenFile)
+		shellPlanFile := filepath.Join(baseDir, "shell_plan.json")


change the name of the test case above to t.Run(baseDir+"_testShell, ... ?

savil · 2022-10-12T00:35:06Z

devbox_test.go

+	require.New(t).NoError(err)
+
+	baseDir := filepath.Dir(testPath)
+	t.Run(baseDir, func(t *testing.T) {


change name to t.Run(baseDir+"_testBuild" + testPath, ... ?

savil · 2022-10-12T00:41:42Z

pkgslice/slice.go

@@ -30,3 +30,17 @@ func Exclude(s []string, elems []string) []string {
 	}
 	return filtered
 }
+
+// returns true when s includes all elements from elems.


this is okay, but thought I'd mention lo.Contains as alternative way
https://github.com/samber/lo#contains

I saw you importing this package lo in #230. If we need it in multiple places, I can switch to use that

savil · 2022-10-12T03:19:05Z

planner/languages/php/php_planner.go

-	plan := &plansdk.Plan{
+	return &plansdk.ShellPlan{
+		Definitions: p.definitions(srcDir, v),
+	}


is a user able to specify the definitions they want to use, if the default-planner one is not correct for them?

No, not right now. Though, this is not a regression because we have that problem before.

savil · 2022-10-12T03:30:52Z

pkgslice/slice.go

@@ -30,3 +30,17 @@ func Exclude(s []string, elems []string) []string {
 	}
 	return filtered
 }
+
+// returns true when s includes all elements from elems.
+func Contains(s []string, elems []string) bool {


for my sanity:
could we call s as superset and elems as subset ?

savil · 2022-10-12T03:56:24Z

planner/plansdk/plansdk.go

-	packagesPlan, err := MergePlans(userPlan, automatedPlan)
+	packagesPlan := &BuildPlan{
+		DevPackages:     append([]string{}, userPlan.DevPackages...),
+		RuntimePackages: []string{},


should we also add NixOverlays here since both userPlan and automatedPlan might specify that?

AppendWithSlice should handle that case properly

savil · 2022-10-12T04:08:18Z

My suggestion as a path forward to land this quickly (which I do want for the shellPlanner versus buildPlanner separation) is to add back to the ShellPlanners the DevPackages for each language. What do you think @LucilleH ?

Then we can independently iron out the product story around (possibly) dropping the auto-add of packages from the planners. As is, I fear we'll break functionality for quite a few users.

LucilleH · 2022-10-13T14:32:17Z

My suggestion as a path forward to land this quickly (which I do want for the shellPlanner versus buildPlanner separation) is to add back to the ShellPlanners the DevPackages for each language. What do you think @LucilleH ?

Since shell plans are only selected when users have all of the packages a shell plan expect, user defined packages will always include devPackages from shell plan @savil
See bullet 2 from PR description

savil

nice! Lets ship it!

Shell plan is applied when user defined packages include all of the packages the shell plan requires include one of the packages from the shell plan.

This could get tricky in some situations. For a C++ project, the user may add gcc package.

But this would then activate the Rust shell planner (which includes two packages, a rust pkg and "gcc"). We could maybe tighten this up in a follow up.

savil · 2022-10-14T21:09:21Z

planner/languages/rust/rust_planner.go

+	}
+
+	rustPkgDev := fmt.Sprintf("rust-bin.stable.%s.default", rustVersion)
+	plan.DevPackages = []string{rustPkgDev, "gcc"}


A style nit is that its easier to track what fields are filled in the ShellPlan struct if we construct it at the end, as in:

manifest, err := ... rustVersion, err := ... rustPkgDev := fmt.Sprintf(.... return &plansdk.ShellPlan { DevPackages: []string{rustPkgDev, "gcc"} NixOverlays: ... }

Need to return the nix overlay on error cases, which is why this is formatted this way. Thoughts?

savil · 2022-10-14T21:11:13Z

planner/planner.go

-			// if merge fails, we return no errors for now.
+	if len(userPkgs) == 0 {
+		// Since user have no packages added yet, we merge all applicable shell planners
+		// And make proper suggestion accordingly.


where is this suggestion made?

In devbox.go, ShellPlan()

LucilleH · 2022-10-17T14:38:25Z

But this would then activate the Rust shell planner (which includes two packages, a rust pkg and "gcc"). We could maybe tighten this up in a follow up.

Rust shell planner will only be applied when it is relevant (cargo.toml file exist), so I think this is ok?

LucilleH force-pushed the lucille--shell-struct branch from dad4793 to 1bc8439 Compare October 10, 2022 18:33

LucilleH requested review from loreto and savil and removed request for loreto October 10, 2022 18:37

savil mentioned this pull request Oct 11, 2022

[shell] minimal revert of #222 to bring back shell plans temporarily #228

Merged

1 task

savil reviewed Oct 12, 2022

View reviewed changes

LucilleH force-pushed the lucille--shell-struct branch 2 times, most recently from a1126e9 to 62cb346 Compare October 13, 2022 15:07

LucilleH requested a review from savil October 13, 2022 15:18

LucilleH force-pushed the lucille--shell-struct branch 4 times, most recently from 3a50d46 to 2614d98 Compare October 14, 2022 17:47

savil approved these changes Oct 14, 2022

View reviewed changes

LucilleH added 4 commits October 17, 2022 09:47

Separate build and shell struct

409065e

Gate planners by user defined packages

6eed85a

Consolidate buildPlan.Invalid and buildPlan.Buildable

f96e05f

address comments

dc01aa2

LucilleH force-pushed the lucille--shell-struct branch 3 times, most recently from 36bdb66 to 8c4c7e3 Compare October 17, 2022 20:21

Suggest extra packages that the users may need

ba4a3b7

LucilleH force-pushed the lucille--shell-struct branch from 8c4c7e3 to ba4a3b7 Compare October 17, 2022 20:24

LucilleH merged commit 0faa80b into main Oct 17, 2022

LucilleH deleted the lucille--shell-struct branch October 17, 2022 20:40

savil mentioned this pull request Oct 17, 2022

[RFC] add config packages to BuildPlan's runtimePackages #234

Closed

Lagoja mentioned this pull request Oct 18, 2022

Feature Request: ability to turn off autodetection languages/stacks #224

Closed

Lagoja linked an issue Oct 18, 2022 that may be closed by this pull request

Feature Request: ability to turn off autodetection languages/stacks #224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[planners] Separate shell plan from build plan #227

[planners] Separate shell plan from build plan #227

LucilleH commented Oct 10, 2022 •

edited

Loading

savil left a comment

savil Oct 12, 2022

LucilleH Oct 13, 2022

savil Oct 12, 2022

LucilleH Oct 13, 2022

savil Oct 12, 2022

LucilleH Oct 13, 2022 •

edited

Loading

savil Oct 12, 2022

savil Oct 12, 2022

savil Oct 12, 2022

LucilleH Oct 13, 2022

savil Oct 12, 2022

LucilleH Oct 13, 2022 •

edited

Loading

savil Oct 12, 2022

savil Oct 12, 2022

LucilleH Oct 13, 2022

savil commented Oct 12, 2022

LucilleH commented Oct 13, 2022 •

edited

Loading

savil left a comment

savil Oct 14, 2022

LucilleH Oct 17, 2022 •

edited

Loading

savil Oct 14, 2022

LucilleH Oct 17, 2022

LucilleH commented Oct 17, 2022

[planners] Separate shell plan from build plan #227

[planners] Separate shell plan from build plan #227

Conversation

LucilleH commented Oct 10, 2022 • edited Loading

Summary

How was it tested?

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucilleH Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucilleH Oct 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savil commented Oct 12, 2022

LucilleH commented Oct 13, 2022 • edited Loading

savil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucilleH Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucilleH commented Oct 17, 2022

LucilleH commented Oct 10, 2022 •

edited

Loading

LucilleH Oct 13, 2022 •

edited

Loading

LucilleH Oct 13, 2022 •

edited

Loading

LucilleH commented Oct 13, 2022 •

edited

Loading

LucilleH Oct 17, 2022 •

edited

Loading