[devbox.json] Allowing including plugins explicitly via "include" field. #986

mikeland73 · 2023-05-05T21:39:11Z

Summary

This adds the ability to include plugins explicitly via:

 "include": [
    "plugin:php"
  ]

in devbox.json. The idea is to allow including any devbox.json into an existing one. The proposed syntax is:

path: for local files
https:// for remote files
plugin: for built-in plugins

only plugin is implemented in this PR.

Improved existing plugin mechanism by using lockfile to decide is devbox.d directory needs to be created by keeping track of the plugin version. In the future we could allow upgrading of configs if there is version mismatch.

A few design questions: (cc: @Lagoja @loreto )

Should include be merged into packges instead? We could introspect the include to see if it's a flake or a plugin. This would simplify the devbox.json but it might be a weird way to include other configs.
One kinda ugly thing of current design is that the php plugin doesn't actually install PHP. That feels a bit wrong, but the way we've currently designed plugins is that they trigger on packages so not sure how to improve this. One option is to rename builtin to something that makes it more explicit that it extends functionality on existing nix packages. Maybe extend:php. Another option: In the future I expect includes to contain packages which would be installed as well. So could parametrize the php plugin so it can take the top level php package as an input (but otherwise it could install its own)

Edit:

After chatting with @loreto and @Lagoja we agreed to keep this design with following:

replace "builtin" with "plugin".
Including plugins works the same way current plugins. Specifically init hooks, and environment get added to existing environment. No new packages are installed.

How was it tested?

In php flake example, did devbox services up

loreto · 2023-05-09T15:12:59Z

I like the functionality, but I'm unsure of the interface/schema in devbox.json (mainly due to similar concerns to the product questions you are raising). Worth chatting in person to flesh the interface out?

Lagoja · 2023-05-09T21:27:24Z

Should include be merged into packges instead? We could introspect the include to see if it's a flake or a plugin. This would simplify the devbox.json but it might be a weird way to include other configs.

I lean towards not including this in packages for now. While it might simplify the config, plugins and other devbox.json configs feel like a separate thing from packages (which ultimately tie back to a Nix package, either from a Flake or the nixpkgs repo). Having it in includes is a bit clearer.

One kinda ugly thing of current design is that the php plugin doesn't actually install PHP. That feels a bit wrong, but the way we've currently designed plugins is that they trigger on packages so not sure how to improve this. One option is to rename builtin to something that makes it more explicit that it extends functionality on existing nix packages. Maybe extend:php. Another option: In the future I expect includes to contain packages which would be installed as well. So could parametrize the php plugin so it can take the top level php package as an input (but otherwise it could install its own)

I think we might want to rename the plugin from php to something else? Maybe being explicit and calling it builtin:php-plugin or builtin:php-config might make this clearer. Based on the answer above, we should make sure these are distinct from packages

Happy to chat about this in person to flesh out.

savil · 2023-05-09T21:57:09Z

suggestion: avoid calling it builtin, because that usually implies a concept that ships with devbox. For example, in programming languages builtin functions ship with the language runtime. In our case, we do envision a world where plugins could be written by users, and those would not be "builtin".

Alternatives: "plugin: php", or "extension: php-plugin".

mikeland73 · 2023-05-09T22:14:56Z

suggestion: avoid calling it builtin, because that usually implies a concept that ships with devbox

Yep, same page. This is precisely why I called them builtin. builtin can only be used with plugins in our plugin directory which are compiled into devbox. For any other plugins, they would be referred to as path or https URLs that would point to a relative local path or a remote address.

savil

I have one concern with the implementation on plugin/plugin.go:245. I think it may be backwards incompatible: it may lead to undesirable overwriting of files for existing devbox-projects. WDYT?

How does a user discover which plugins are available?

(short term) Could we have a docs page that we reference in the help notes?
(long term) implement devbox plugins ls?

Including plugins works the same way current plugins. Specifically init hooks, and environment get added to existing environment. No new packages are installed.

what is the behavior a user would experience if they have includes: [ plugins: php ] but no package php?

savil · 2023-05-11T18:02:58Z

internal/impl/config.go

+	// path: for local files
+	// https:// for remote files
+	// plugin: for built-in plugins
+	// This is similar to nix inputs


Mind elaborating on what you mean by "similar to nix inputs": in what way?

nix flake inputs are urls of this format (path, https, github, etc) so we are using the same format.

I see, got it. Perhaps "This format is similar to nix inputs"

savil · 2023-05-11T18:13:51Z

internal/impl/packages.go

 		return err
 	}

-	d.pluginManager.ApplyOptions(plugin.WithAddMode())


when does this get applied now, in the flow for adding a package?

Oh I understand this one. The AddMode is replaced with Lockfile.

savil · 2023-05-11T18:23:00Z

internal/plugin/plugin.go

-	// Only create devboxDir files in add mode.
-	if strings.Contains(filePath, devboxDirName) && !m.addMode {
+func (m *Manager) shouldCreateFile(pkg *lock.Package, filePath string) bool {
+	// Only create files in devboxDir is they are not in the lockfile


🤔 for older plugins (i.e. not in the lockfile), this code is re-creating their devbox.d files.

I thought users were encouraged to commit them to source-control and possibly change them.

so, shouldn't we avoid re-generating these files for existing devbox projects

I'll make sure we don't overwrite them, but yeah this will cause a one time re-creation if they previously deleted/moved them. I think that's on ok tradeoff in order to migrate to a better system.

Got it

Yeah, I was concerned about the overwriting scenario where users may have made changes.

savil · 2023-05-11T18:31:02Z

internal/impl/generate.go

+		if err := d.lockfile.Add(includes); err != nil {
+			return err
+		}
+		if err := d.pluginManager.Include(d.writer, includes, d.projectDir); err != nil {


It'll be good for some future PR to make the plugin manager life-cycle more clear.

Suggestion:

pluginManager.New(lockfile) function that does applyOptions

pluginManager.Generate(packages, includes)

this can do lockfile.Add(includes) internally

NewManager already applies options. The reason we call it separately is because of a circular dependency (see impl.Open function)

Agree with unifying Create and Includes

Not sure about calling lockfile.Add(includes) internally. That feels like logic that is unrelated to plugins. Insterad, plugins should be saved to lock file and then we can pass in resolved packages directly to plugin manager, which will mean we no longer need the lockfile in there.

NewManager already applies options. The reason we call it separately is because of a circular dependency (see impl.Open function)

Oh I see. I left a comment above about improving that so its easier to reason about.

Not sure about calling lockfile.Add(includes) internally. That feels like logic that is unrelated to plugins. Insterad, plugins should be saved to lock file and then we can pass in resolved packages directly to plugin manager, which will mean we no longer need the lockfile in there.

Yup, you're right. Lets not call lockfile.Add from within pluginManager. 👍🏾

savil · 2023-05-11T18:31:49Z

internal/impl/generate.go

+		}
+	}
+
+	for _, includes := range d.cfg.Include {


nit: shouldn't this be singular include?

actually, in addition, I'm being thrown off by d.cfg.Include being singular....should that be plural?

Relatedly, in the devbox.json, should the field be includes?

"include" is a verb, not a noun so I think it should be singular (e.g. tsconfig.json also uses "include" and not "includes"). @Lagoja curious if you have opinion.

For the range variable, I'll change it, maybe to included?

oh its a verb! I see i see.
Okay, we can leave as is for devbox.json, and yeah I like included for the range var 👍🏾

savil · 2023-05-12T00:42:37Z

internal/impl/config.go

+	// path: for local files
+	// https:// for remote files
+	// plugin: for built-in plugins
+	// This is similar to nix inputs


I see, got it. Perhaps "This format is similar to nix inputs"

savil · 2023-05-12T00:44:15Z

internal/impl/generate.go

+		}
+	}
+
+	for _, includes := range d.cfg.Include {


actually, in addition, I'm being thrown off by d.cfg.Include being singular....should that be plural?

Relatedly, in the devbox.json, should the field be includes?

savil · 2023-05-12T00:46:53Z

internal/impl/devbox.go

 	if err != nil {
 		return nil, err
 	}
+	box.pluginManager.ApplyOptions(plugin.WithLockfile(lock))


I'd suggest doing:

remove pluginmanager: plugin.NewManager(), above

make this box.pluginManager = plugin.NewManager(plugin.WithLockfile(lock))

make applyOptions an internal function,

(3) would make NewManager a more genuine constructor. That would make it easier to reason about that all the pluginManager's fields are ready and methods can be applied.

savil · 2023-05-12T00:50:32Z

internal/impl/generate.go

+		if err := d.lockfile.Add(includes); err != nil {
+			return err
+		}
+		if err := d.pluginManager.Include(d.writer, includes, d.projectDir); err != nil {


NewManager already applies options. The reason we call it separately is because of a circular dependency (see impl.Open function)

Oh I see. I left a comment above about improving that so its easier to reason about.

Not sure about calling lockfile.Add(includes) internally. That feels like logic that is unrelated to plugins. Insterad, plugins should be saved to lock file and then we can pass in resolved packages directly to plugin manager, which will mean we no longer need the lockfile in there.

Yup, you're right. Lets not call lockfile.Add from within pluginManager. 👍🏾

savil · 2023-05-12T00:51:18Z

internal/plugin/plugin.go

-	// Only create devboxDir files in add mode.
-	if strings.Contains(filePath, devboxDirName) && !m.addMode {
+func (m *Manager) shouldCreateFile(pkg *lock.Package, filePath string) bool {
+	// Only create files in devboxDir is they are not in the lockfile


Got it

Yeah, I was concerned about the overwriting scenario where users may have made changes.

mikeland73 · 2023-05-15T05:16:17Z

How does a user discover which plugins are available?

(short term) Could we have a docs page that we reference in the help notes?

Built in plugin functionality is already documented. Plugins are not new, they were just only included by package whereas this allows them to be included explicitly.

(long term) implement devbox plugins ls?

Maybe. There's also a world where plugins are as ubiquitous as packages, and most of them are third party. So a search might be more appropriate than an ls.

what is the behavior a user would experience if they have includes: [ plugins: php ] but no package php?

The service will fail. For other plugins the init hooks will fail. I think this is OK. The match mechanism in built in plugins is a bit too magical, and I'm not sure we should expand it to check if you have correct packages installed. Not sure.

mikeland73 · 2023-05-15T18:21:37Z

@savil I confirmed that files in devbox.d do not get replaced if they already exists (This is existing functionality)

mikeland73 added 2 commits April 27, 2023 13:49

[WIP] config includes

c13527d

Merge branch 'main' into landau/includes

920e0c0

mikeland73 changed the title ~~Landau/includes~~ [WIP] config includes May 5, 2023

mikeland73 added 5 commits May 5, 2023 14:41

Revert some changes

7456087

Merge branch 'main' into landau/includes

6c00e01

Hook up includes to new lockfile

c214e3e

Revert bad change

aa21d40

Fixed plugin env, added comments

e730ea9

mikeland73 requested review from savil and ipince May 8, 2023 23:50

mikeland73 marked this pull request as ready for review May 9, 2023 00:07

mikeland73 changed the title ~~[WIP] config includes~~ Config includes May 9, 2023

mikeland73 added 2 commits May 10, 2023 12:37

Merge branch 'main' into landau/includes

0687263

Rename builtin -> plugin

6b4a86a

mikeland73 changed the title ~~Config includes~~ [devbox.json] Allowing including plugins explicitly via "include" field. May 10, 2023

savil reviewed May 11, 2023

View reviewed changes

savil reviewed May 12, 2023

View reviewed changes

Merge branch 'main' into landau/includes

e6b7e5e

mikeland73 added 2 commits May 15, 2023 11:09

Merge branch 'main' into landau/includes

98cf16a

Requested copy change

5b14b97

savil approved these changes May 15, 2023

View reviewed changes

mikeland73 merged commit e2478a2 into main May 15, 2023

mikeland73 deleted the landau/includes branch May 15, 2023 18:47

snyk-io bot mentioned this pull request Oct 23, 2024

[Snyk] Fix for 1 vulnerabilities SherfeyInv/devbox#87

Closed

[devbox.json] Allowing including plugins explicitly via "include" field. #986

[devbox.json] Allowing including plugins explicitly via "include" field. #986

Uh oh!

Conversation

mikeland73 commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How was it tested?

Uh oh!

loreto commented May 9, 2023

Uh oh!

Lagoja commented May 9, 2023

Uh oh!

savil commented May 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mikeland73 commented May 9, 2023

Uh oh!

savil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeland73 commented May 15, 2023

Uh oh!

mikeland73 commented May 15, 2023

Uh oh!

Uh oh!

mikeland73 commented May 5, 2023 •

edited

Loading

savil commented May 9, 2023 •

edited

Loading