[Config Packages] Enable optional map spec #1351

savil · 2023-08-07T20:59:22Z

Summary

Motivation
User feedback is leading us to add more parameters besides version to the packages config.
For instance, some packages may only need to be added on certain platforms.

Previously, packages were specified as []string. For example:

{
  "packages": [
    "[email protected]",
    "hello",
    "cowsay@latest"
  ],
  ...
}

In the new format, they can also be specified as:

{
   "packages": {
      "python": "3.9",
      "hello": "",
      "cowsay": {
        "version": "latest",
      }
   }
}

NOTE: python is an inline-version, while cowsay has a struct value which can
accomodate other fields like platforms.

Approach
For now, we enable both packages as List and Map to work. The default continues to be List.
We do this by adding a Packages struct and Package struct with custom JSON marshalling and unmarshalling.

Future PRs

Introduce Packages.platforms and/or Packages.excludedPlatforms fields. Use them when installing to nix profile, and generating the flake.nix.
Introduce devbox add and devbox rm flags for platform-specific packages.
Detect package usage failures when due to platform mis-match and suggest using CLI commands to run to make platform-specific.

How was it tested?

Added TestJsonifyConfigPackages and other unit-tests

testscripts pass

devbox init produces the same output as before.

savil · 2023-08-07T20:59:35Z

Current dependencies on/for this PR:

main
- PR remove cuego.Complete from cuecfg.Marshal #1349
  - PR [Config Packages] Enable optional map spec #1351 👈

This comment was auto-generated by Graphite.

mikeland73

This is really nice implementation! great encapsulation, super easy to read.

mikeland73 · 2023-08-07T22:46:32Z

internal/devconfig/packages.go

+ }
+
+ // If we have a regular package, we want to marshal the entire struct:
+ type Alias Package // Use an alias-type to avoid infinite recursion


This is cool! I've gotten into infinite recursion issue when implementing custom marshaling and I don't remember the solution being this clean.

nit, make lower case

thanks, and lower cased

mikeland73 · 2023-08-07T22:49:18Z

internal/devconfig/packages.go

+}
+
+// parseVersionedName parses the name and version from package@version representation
+func parseVersionedName(versionedName string) (name, version string) {


see searcher.ParseVersionedPackage

this does a little more, but hopefully you can use it to prevent some code duplication.

ohh good call. Yes, I can use it. There are some subtle differences so I still need this function here, but can call into searcher.ParseVersionedPackage in the implementation.

I've augmented the searcher.ParseVersionedPackage to handle a couple cases I do here. And added test-cases.

mikeland73 · 2023-08-07T22:51:37Z

internal/devconfig/packages.go

+ type Alias Package // Use an alias-type to avoid infinite recursion
+ alias := &Alias{}


nit lower case, maybe

type packageAlias Package // Use an alias-type to avoid infinite recursion alias := &packageAlias{}

gcurtis

Thanks for the tests! I also like the idea of aliasing the type for (un)marshalling.

The map order mattering is unfortunate. Maybe we can eventually stop relying on that.

gcurtis · 2023-08-08T02:33:39Z

internal/devconfig/packages.go

+// Remove removes a package from the list of packages
+func (pkgs *Packages) Remove(versionedName string) error {
+ name, version := parseVersionedName(versionedName)
+ for idx, pkg := range pkgs.Collection {


slices.DeleteFunc might be a good fit here.

nice! done.

gcurtis · 2023-08-08T02:38:33Z

internal/devconfig/packages.go

+ orderedmap "github.com/wk8/go-ordered-map/v2"
+)
+
+type jsonKind int


Could reuse json.Delim.

leaving in place for now, since it feels cleaner.

internal/devconfig/packages.go

gcurtis · 2023-08-08T02:42:49Z

internal/devconfig/packages.go

+
+func (pkgs *Packages) UnmarshalJSON(data []byte) error {
+
+ // First, attempt to unmarshal as a list of strings (legacy format)


You could also check the first byte for '[' or '{'.

leaving in place, since its cleaner

gcurtis · 2023-08-08T02:46:32Z

go.mod

@@ -38,13 +38,21 @@ require (
 github.com/spf13/cobra v1.6.1
 github.com/spf13/pflag v1.0.5
 github.com/stretchr/testify v1.8.2
+ github.com/wk8/go-ordered-map/v2 v2.1.8


Why are there two orderedmap dependencies?

oh I added the iancoleman one first, and then switched. This wk8 one is more modern and has a nicer API.

I'll remove the iancoleman one.

gcurtis · 2023-08-08T02:48:53Z

internal/devconfig/packages.go

+ // We use orderedmap to preserve the order of the packages. While the JSON
+ // specification specifies that maps are unordered, we do rely on the order
+ // for certain functionality.
+ orderedMap := orderedmap.New[string, Package]()


You could also use a json.Decoder to preserve the order and assign the key to the name field as you go.

In the interests of time, I'm going to keep orderedmap for now. It seems to work well, and has a clean api.

The other library I used (see here) used json.Decoder in its implementation and its fairly non-trivial.

gcurtis · 2023-08-08T02:50:13Z

internal/devconfig/packages.go

+
+func (pkgs *Packages) MarshalJSON() ([]byte, error) {
+ if pkgs.jsonKind == jsonList {
+ packagesList := []string{}


Suggested change

packagesList := []string{}

packagesList := make([]string, 0, len(pkgs.Collection))

gcurtis · 2023-08-08T03:07:44Z

internal/devconfig/packages.go

+ return errors.WithStack(err)
+ }
+
+ // more robust way to copy all fields from alias?


Try *p = Package(*alias)

ahhh thanks!!! Didn't know there was a built-in struct-copy function

… devconfig.parseVersionedName

savil

Thanks for the comments! responses inline, and updates made.

savil · 2023-08-08T15:49:46Z

go.mod

@@ -38,13 +38,21 @@ require (
 github.com/spf13/cobra v1.6.1
 github.com/spf13/pflag v1.0.5
 github.com/stretchr/testify v1.8.2
+ github.com/wk8/go-ordered-map/v2 v2.1.8


oh I added the iancoleman one first, and then switched. This wk8 one is more modern and has a nicer API.

I'll remove the iancoleman one.

savil · 2023-08-08T17:48:02Z

internal/devconfig/packages.go

+// Remove removes a package from the list of packages
+func (pkgs *Packages) Remove(versionedName string) error {
+ name, version := parseVersionedName(versionedName)
+ for idx, pkg := range pkgs.Collection {


nice! done.

savil · 2023-08-08T17:51:06Z

internal/devconfig/packages.go

+
+func (pkgs *Packages) MarshalJSON() ([]byte, error) {
+ if pkgs.jsonKind == jsonList {
+ packagesList := []string{}


savil · 2023-08-08T17:52:24Z

internal/devconfig/packages.go

+ // We use orderedmap to preserve the order of the packages. While the JSON
+ // specification specifies that maps are unordered, we do rely on the order
+ // for certain functionality.
+ orderedMap := orderedmap.New[string, Package]()


In the interests of time, I'm going to keep orderedmap for now. It seems to work well, and has a clean api.

The other library I used (see here) used json.Decoder in its implementation and its fairly non-trivial.

savil · 2023-08-08T17:54:05Z

internal/devconfig/packages.go

+ type Alias Package // Use an alias-type to avoid infinite recursion
+ alias := &Alias{}


savil · 2023-08-08T17:56:17Z

internal/devconfig/packages.go

+ }
+
+ // If we have a regular package, we want to marshal the entire struct:
+ type Alias Package // Use an alias-type to avoid infinite recursion


thanks, and lower cased

savil · 2023-08-08T18:35:05Z

internal/devconfig/packages.go

+}
+
+// parseVersionedName parses the name and version from package@version representation
+func parseVersionedName(versionedName string) (name, version string) {


ohh good call. Yes, I can use it. There are some subtle differences so I still need this function here, but can call into searcher.ParseVersionedPackage in the implementation.

I've augmented the searcher.ParseVersionedPackage to handle a couple cases I do here. And added test-cases.

internal/devconfig/packages.go

savil · 2023-08-08T18:35:53Z

internal/devconfig/packages.go

+ orderedmap "github.com/wk8/go-ordered-map/v2"
+)
+
+type jsonKind int


leaving in place for now, since it feels cleaner.

savil · 2023-08-08T18:40:10Z

internal/devconfig/packages.go

+
+func (pkgs *Packages) UnmarshalJSON(data []byte) error {
+
+ // First, attempt to unmarshal as a list of strings (legacy format)


leaving in place, since its cleaner

savil mentioned this pull request Aug 7, 2023

remove cuego.Complete from cuecfg.Marshal #1349

Merged

3 tasks

Base automatically changed from savil/rm-cuego-complete to main August 7, 2023 21:02

savil force-pushed the savil/config-packages branch 4 times, most recently from 93e1315 to 22fa857 Compare August 7, 2023 22:11

savil requested review from gcurtis, mikeland73 and ipince August 7, 2023 22:15

savil marked this pull request as ready for review August 7, 2023 22:16

mikeland73 approved these changes Aug 7, 2023

View reviewed changes

gcurtis approved these changes Aug 8, 2023

View reviewed changes

savil added 5 commits August 8, 2023 11:37

[Config Packages] Enable optional map spec

df29e2a

switch to more modern ordered map library

342c2e0

remove iancoleman/orderedmap dependency via go mod tidy

bf03a38

use slices.DeleteFunc, and some minor fixes

08e9a9f

improve searcher.ParseVersionedPackage, and reuse in implemetation of…

c806c1f

… devconfig.parseVersionedName

savil force-pushed the savil/config-packages branch from 6e6e9bd to c806c1f Compare August 8, 2023 18:37

lintfix

962ca3c

savil commented Aug 8, 2023

View reviewed changes

savil merged commit 1b5ece8 into main Aug 8, 2023
15 checks passed

savil deleted the savil/config-packages branch August 8, 2023 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Config Packages] Enable optional map spec #1351

[Config Packages] Enable optional map spec #1351

savil commented Aug 7, 2023 •

edited

Loading

savil commented Aug 7, 2023

mikeland73 left a comment

mikeland73 Aug 7, 2023

savil Aug 8, 2023

mikeland73 Aug 7, 2023

savil Aug 8, 2023

mikeland73 Aug 7, 2023

savil Aug 8, 2023

gcurtis left a comment

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

gcurtis Aug 8, 2023

savil Aug 8, 2023

savil left a comment

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

savil Aug 8, 2023

		type Alias Package // Use an alias-type to avoid infinite recursion
		alias := &Alias{}


		func (pkgs *Packages) UnmarshalJSON(data []byte) error {

		// First, attempt to unmarshal as a list of strings (legacy format)

	packagesList := []string{}
	packagesList := make([]string, 0, len(pkgs.Collection))

[Config Packages] Enable optional map spec #1351

[Config Packages] Enable optional map spec #1351

Conversation

savil commented Aug 7, 2023 • edited Loading

Summary

How was it tested?

savil commented Aug 7, 2023

mikeland73 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

Choose a reason for hiding this comment

gcurtis 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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 Aug 7, 2023 •

edited

Loading