Skip to content

[per-OS Packages] in devbox add, allow multiple --platform/--exclude-platform flag values #1363

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.

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

Merged
merged 3 commits into from
Aug 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion devbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ type Devbox interface {
// Add adds Nix packages to the config so that they're available in the devbox
// environment. It validates that the Nix packages exist, and install them.
// Adding duplicate packages is a no-op.
Add(ctx context.Context, platform, excludePlatform string, pkgs ...string) error
Add(ctx context.Context, platforms, excludePlatforms []string, pkgs ...string) error
Config() *devconfig.Config
ProjectDir() string
// Generate creates the directory of Nix files and the Dockerfile that define
Expand Down
18 changes: 9 additions & 9 deletions internal/boxcli/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ import (
const toSearchForPackages = "To search for packages, use the `devbox search` command"

type addCmdFlags struct {
config configFlags
allowInsecure bool
platform string
excludePlatform string
config configFlags
allowInsecure bool
platforms []string
excludePlatforms []string
}

func addCmd() *cobra.Command {
Expand Down Expand Up @@ -53,11 +53,11 @@ func addCmd() *cobra.Command {
command.Flags().BoolVar(
&flags.allowInsecure, "allow-insecure", false,
"allow adding packages marked as insecure.")
command.Flags().StringVar(
&flags.platform, "platform", "",
command.Flags().StringSliceVarP(
&flags.platforms, "platform", "p", []string{},
"add packages to run on only this platform.")
command.Flags().StringVar(
&flags.excludePlatform, "exclude-platform", "",
command.Flags().StringSliceVarP(
&flags.excludePlatforms, "exclude-platform", "e", []string{},
"exclude packages from a specific platform.")

return command
Expand All @@ -73,5 +73,5 @@ func addCmdFunc(cmd *cobra.Command, args []string, flags addCmdFlags) error {
return errors.WithStack(err)
}

return box.Add(cmd.Context(), flags.platform, flags.excludePlatform, args...)
return box.Add(cmd.Context(), flags.platforms, flags.excludePlatforms, args...)
}
65 changes: 24 additions & 41 deletions internal/devconfig/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ package devconfig

import (
"encoding/json"
"os"
"strings"

"github.com/pkg/errors"
"github.com/samber/lo"
orderedmap "github.com/wk8/go-ordered-map/v2"
"go.jetpack.io/devbox/internal/boxcli/usererr"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/searcher"
"go.jetpack.io/devbox/internal/ux"
"golang.org/x/exp/slices"
)

Expand Down Expand Up @@ -59,29 +58,21 @@ func (pkgs *Packages) Remove(versionedName string) {
})
}

// AddPlatform adds a platform to the list of platforms for a given package
func (pkgs *Packages) AddPlatform(versionedname, platform string) error {
if err := nix.EnsureValidPlatform(platform); err != nil {
// AddPlatforms adds a platform to the list of platforms for a given package
func (pkgs *Packages) AddPlatforms(versionedname string, platforms []string) error {
if len(platforms) == 0 {
return nil
}
if err := nix.EnsureValidPlatform(platforms...); err != nil {
return errors.WithStack(err)
}

name, version := parseVersionedName(versionedname)
for idx, pkg := range pkgs.Collection {
if pkg.name == name && pkg.Version == version {

// Check if the platform is already present
alreadyPresent := false
for _, existing := range pkg.Platforms {
if existing == platform {
alreadyPresent = true
break
}
}

// Add the platform if it's not already present
if !alreadyPresent {
pkg.Platforms = append(pkg.Platforms, platform)
}
// Append if the platform is not already present
pkg.Platforms = lo.Uniq(append(pkg.Platforms, platforms...))

// Adding any platform will restrict installation to it, so
// the ExcludedPlatforms are no longer needed
Expand All @@ -96,35 +87,27 @@ func (pkgs *Packages) AddPlatform(versionedname, platform string) error {
return errors.Errorf("package %s not found", versionedname)
}

// ExcludePlatform adds a platform to the list of excluded platforms for a given package
func (pkgs *Packages) ExcludePlatform(versionedName, platform string) error {
if err := nix.EnsureValidPlatform(platform); err != nil {
return errors.WithStack(err)
// ExcludePlatforms adds a platform to the list of excluded platforms for a given package
func (pkgs *Packages) ExcludePlatforms(versionedName string, platforms []string) error {
if len(platforms) == 0 {
return nil
}
for _, platform := range platforms {
if err := nix.EnsureValidPlatform(platform); err != nil {
return errors.WithStack(err)
}
}

name, version := parseVersionedName(versionedName)
for idx, pkg := range pkgs.Collection {
if pkg.name == name && pkg.Version == version {

// Check if the platform is already present
alreadyPresent := false
for _, existing := range pkg.ExcludedPlatforms {
if existing == platform {
alreadyPresent = true
break
}
}

if !alreadyPresent {
pkg.ExcludedPlatforms = append(pkg.ExcludedPlatforms, platform)
}
pkg.ExcludedPlatforms = lo.Uniq(append(pkg.ExcludedPlatforms, platforms...))
if len(pkg.Platforms) > 0 {
ux.Finfo(
os.Stderr,
"Excluding a platform for %[1]s is a bit redundant because it will only be installed on: %[2]v. "+
"Consider removing the `platform` field from %[1]s's definition in your devbox."+
"json if you intend for %[1]s to be installed on all platforms except %[3]s.\n",
versionedName, strings.Join(pkg.Platforms, ", "), platform,
return usererr.New(
"cannot exclude any platform for package %s because it already has `platforms` defined. "+
"Please delete the `platforms` for this package from devbox.json and re-try.",
pkg.VersionedName(),
)
}

Expand Down
14 changes: 5 additions & 9 deletions internal/impl/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ import (
// Add adds the `pkgs` to the config (i.e. devbox.json) and nix profile for this
// devbox project
// nolint:revive // warns about cognitive complexity
func (d *Devbox) Add(ctx context.Context, platform, excludePlatform string, pkgsNames ...string) error {
func (d *Devbox) Add(ctx context.Context, platforms, excludePlatforms []string, pkgsNames ...string) error {
ctx, task := trace.NewTask(ctx, "devboxAdd")
defer task.End()

Expand Down Expand Up @@ -88,15 +88,11 @@ func (d *Devbox) Add(ctx context.Context, platform, excludePlatform string, pkgs
}

for _, pkg := range addedPackageNames {
if platform != "" {
if err := d.cfg.Packages.AddPlatform(pkg, platform); err != nil {
return err
}
if err := d.cfg.Packages.AddPlatforms(pkg, platforms); err != nil {
return err
}
if excludePlatform != "" {
if err := d.cfg.Packages.ExcludePlatform(pkg, excludePlatform); err != nil {
return err
}
if err := d.cfg.Packages.ExcludePlatforms(pkg, excludePlatforms); err != nil {
return err
}
}

Expand Down
2 changes: 1 addition & 1 deletion internal/impl/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (d *Devbox) Update(ctx context.Context, pkgs ...string) error {
// Calling Add function with the original package names, since
// Add will automatically append @latest if search is able to handle that.
// If not, it will fallback to the nixpkg format.
if err := d.Add(ctx, "" /*platform*/, "" /*excludePlatform*/, pkg.Raw); err != nil {
if err := d.Add(ctx, nil /*platforms*/, nil /*excludePlatforms*/, pkg.Raw); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

pain

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we fill these in next PR

return err
}
} else {
Expand Down
21 changes: 15 additions & 6 deletions internal/nix/nix.go
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,24 @@ var nixPlatforms = []string{
"armv7l-linux",
}

// EnsureValidPlatform returns an error if the platform is not supported by nix.
// ensureValidPlatform returns an error if the platform is not supported by nix.
// https://nixos.org/manual/nix/stable/installation/supported-platforms.html
func EnsureValidPlatform(platform string) error {
for _, p := range nixPlatforms {
if p == platform {
return nil
func EnsureValidPlatform(platforms ...string) error {
ensureValid := func(platform string) error {
for _, p := range nixPlatforms {
if p == platform {
return nil
}
}
return usererr.New("Unsupported platform: %s. Valid platforms are: %v", platform, nixPlatforms)
}
return usererr.New("Unsupported platform: %s. Valid platforms are: %v", platform, nixPlatforms)

for _, p := range platforms {
if err := ensureValid(p); err != nil {
return err
}
}
return nil
}

// Warning: be careful using the bins in default/bin, they won't always match bins
Expand Down
56 changes: 55 additions & 1 deletion testscripts/add/add_platforms.test.txt
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Testscript for exercising adding packages

# exec devbox init
#### Part 1: Adding with a single platform or exclude-platform

exec devbox install
! exec rg --version
! exec vim --version
Expand All @@ -17,6 +18,14 @@ exec devbox add vim --exclude-platform x86_64-linux

json.superset devbox.json expected_devbox2.json

#### Part 2: Adding with multiple platforms or exclude-platforms

exec devbox add hello --platform x86_64-darwin,x86_64-linux --platform aarch64-darwin
json.superset devbox.json expected_devbox3.json

exec devbox add cowsay --exclude-platform x86_64-darwin,x86_64-linux --exclude-platform aarch64-darwin
json.superset devbox.json expected_devbox4.json

-- devbox.json --
{
"packages": [
Expand Down Expand Up @@ -52,3 +61,48 @@ json.superset devbox.json expected_devbox2.json
}
}
}

-- expected_devbox3.json --

{
"packages": {
"hello": "",
"cowsay": "latest",
"ripgrep": {
"version": "latest",
"platforms": ["x86_64-darwin", "x86_64-linux"]
},
"vim": {
"version": "latest",
"excluded_platforms": ["x86_64-linux"]
},
"hello": {
"version": "latest",
"platforms": ["x86_64-darwin", "x86_64-linux", "aarch64-darwin"]
}
}
}

-- expected_devbox4.json --

{
"packages": {
"hello": "",
"cowsay": {
"version": "latest",
"excluded_platforms": ["x86_64-darwin", "x86_64-linux", "aarch64-darwin"]
},
"ripgrep": {
"version": "latest",
"platforms": ["x86_64-darwin", "x86_64-linux"]
},
"vim": {
"version": "latest",
"excluded_platforms": ["x86_64-linux"]
},
"hello": {
"version": "latest",
"platforms": ["x86_64-darwin", "x86_64-linux", "aarch64-darwin"]
}
}
}