[per-OS Packages] Add platforms and excluded_platforms functionality #1358

savil · 2023-08-09T18:14:40Z

Summary

Motivation
Some users have a need to install platform-specific packages. For example, glibcLocales cannot be installed on macOS but may be needed on Linux.

Approach
We enable two flags in devbox add:

# install on all platforms except x86_64-darwin
> devbox add glibcLocales --exclude-platform x86_64-darwin

# only install on x86_64-darwin and aarch64-darwin
> devbox add darwin.apple_sdk.frameworks.Security --platform x86_64-darwin 
> devbox add darwin.apple_sdk.frameworks.Security --platform aarch64-darwin

Follow up PRs:

enable multiple --platform flag uses
detect if devbox add fails due to platform-mismatch and suggest --exclude-platform flag

How was it tested?

Added testscript unit test

Adhoc testing:

savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> rm -rf .devbox
savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> devbox shell
Ensuring packages are installed.

Installing package: [email protected].

[1/1] [email protected]
[1/1] [email protected]: Success
Starting a devbox shell...
Welcome to fish, the friendly interactive shell
Type help for instructions on how to use fish

Install for a different platform than the one i am currently running on

(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> devbox add hello --platform x86_64-linux
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> cat devbox.json
{
  "packages": {
    "go": "1.19.8",
    "hello": {
      "version": "latest",
      "platforms": [
        "x86_64-linux"
      ]
    }
  },
  "shell": {
    "init_hook": [
      "export \"GOROOT=$(go env GOROOT)\""
    ],
    "scripts": {
      "run_test": "go run main.go"
    }
  }
}
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> which hello
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2) [1]>

Install on all platforms except my current platform

(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2) [1]> devbox add cowsay --exclude-platform x86_64-darwin
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> which cowsay
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2) [1]> cat devbox.json
{
  "packages": {
    "go": "1.19.8",
    "hello": {
      "version": "latest",
      "platforms": [
        "x86_64-linux"
      ]
    },
    "cowsay": {
      "version": "latest",
      "excluded_platforms": [
        "x86_64-darwin"
      ]
    }
  },
  "shell": {
    "init_hook": [
      "export \"GOROOT=$(go env GOROOT)\""
    ],
    "scripts": {
      "run_test": "go run main.go"
    }
  }
}

Ensure basic adding still works

(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> which vim
/usr/bin/vim
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> devbox add vim

Installing package: vim@latest.

[1/1] vim@latest
[1/1] vim@latest: Success
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> which vim
/Users/savil/code/jetpack/devbox/examples/development/go/hello-world/.devbox/virtenv/.wrappers/bin/vim
(devbox) savil@Savil-Srivastavas-MacBook-Pro ~/c/j/d/e/d/g/hello-world (savil/config-packages-2)> cat devbox.json
{
  "packages": {
    "go": "1.19.8",
    "hello": {
      "version": "latest",
      "platforms": [
        "x86_64-linux"
      ]
    },
    "cowsay": {
      "version": "latest",
      "excluded_platforms": [
        "x86_64-darwin"
      ]
    },
    "vim": "latest"
  },
  "shell": {
    "init_hook": [
      "export \"GOROOT=$(go env GOROOT)\""
    ],
    "scripts": {
      "run_test": "go run main.go"
    }
  }
}

savil · 2023-08-09T18:14:49Z

Current dependencies on/for this PR:

main
- PR [Packages] Introduce the concept of Installable packages #1357
  - PR devbox.findPackagesByName should be constructed from Config packages #1360
    - PR [per-OS Packages] Add platforms and excluded_platforms functionality #1358 👈
      - PR [per-OS Packages] in devbox add, allow multiple --platform/--exclude-platform flag values #1363
        
        PR [per-OS Packages] Suggest --platform/--exclude-platform upon error with installing package that is uninstallable on user's platform #1364
        
        PR [per-OS Packages] fix devbox update for unversioned packages with platform/excluded-platform #1370

This comment was auto-generated by Graphite.

savil · 2023-08-10T14:37:20Z

On second thought, this needs a fix: we should add package to the lockfile, even if not installing on the user's platform.

savil · 2023-08-10T21:46:39Z

internal/devconfig/packages.go

+			// 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)
+			}


this is verbose I realize. In the next PR, it gets simplified to:

if !lo.SomeBy(pkg.Platforms, func(p string) bool { return p == platform }) { pkg.Platforms = append(pkg.Platforms, platform) }

mikeland73 · 2023-08-11T20:01:47Z

internal/boxcli/add.go

+	platform        string
+	excludePlatform string


Should be slice?

yup, done in next PR

mikeland73 · 2023-08-11T20:08:50Z

internal/impl/packages.go

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


Can this be part of d.cfg.Packages.Add(packageNameForConfig) ?

This is a good idea. I'll do it in a stack PR (#1363) to minimize conflicts.

actually, moving this won't work.

We need this here to enable the following scenario:

devbox add hello --platform x86_64-linux devbox add hello --platform x86-64-darwin

The second time it runs, it willl not be within the loop above where .Add() is called.

mikeland73 · 2023-08-11T20:12:04Z

internal/devconfig/packages.go

+// If the package has a list of platforms, it is enabled only on those platforms.
+// If the package has a list of excluded platforms, it is enabled on all platforms
+// except those.
+func (p *Package) enabledOnPlatform(platform string) bool {


can this just be part of IsEnabledOnPlatform ?

mikeland73 · 2023-08-11T20:13:54Z

internal/nix/nix.go

+// 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 {


lo.Contains ? or just use a map.

I find current version simpler to write and read

mikeland73 · 2023-08-11T20:15:45Z

internal/impl/packages.go

+	cfgPkgs, err := d.ConfigPackages()
+	if err != nil {
+		return err
+	}
+	for _, pkg := range cfgPkgs {
+		if err := pkg.EnsureUninstallableIsInLockfile(); err != nil {
+			return err
+		}
+	}


EnsureUninstallableIsInLockfile is a bit awkward, you could simply loop on d.ConfigPackages() and resolve all.

If they are already resolved, it's a no-op.

it has some more checks inside the function....

## Summary This PR lays some groundwork for introducing platform-specific packages in a later PR (#1358) This PR makes the following changes to `devbox` struct: 1. Replace `devbox.Packages` with `ConfigPackageNames()` and `InstallablePackageNames()`. 2. Replace `devbox.PackagesAsInputs` with `ConfigPackages()` and `InstallablePackages()`. Updates callers to call the appropriate one. **devpkg.Package changes** I also introduce a new method `devpkg.PackagesFromConfig`, which enables us to add an `isInstallable bool` field on the `devpkg.Package`. I needed this because it affects the implementation of certain methods (such as `IsLegacy` changed in #1358). One can still construct via `PackagesFromStrings` which default to `isInstallable = true` (as before). I audited the callers. ## How was it tested? Did adhoc testing. Testscript unit tests should pass.

…ge.IsLegacy change

…her direct devbox library callers)

savil · 2023-08-12T00:28:37Z

test failure is spurious. Tests pass in subsequent PRs.

sentry-io · 2023-08-14T15:43:52Z

Suspect Issues

This pull request has been deployed and Sentry observed the following issues:

‼️ **exec.ExitError: <redacted *errors.withStack>: <redacted errors.withMessage>: <redacted exec.ExitError> go.jetpack.io/devbox/internal/nix in PrintDevEnv View Issue

_{Did you find this useful? React with a 👍 or 👎}

savil mentioned this pull request Aug 9, 2023

[Packages] Introduce the concept of Installable packages #1357

Merged

savil force-pushed the savil/config-packages-2 branch 2 times, most recently from 37eafa8 to 215050c Compare August 9, 2023 22:35

savil force-pushed the savil/config-pkg-to-devpkg branch from 9a6c804 to 485ec44 Compare August 9, 2023 23:06

savil force-pushed the savil/config-packages-2 branch 2 times, most recently from 6f3367e to 94a01fa Compare August 9, 2023 23:49

savil force-pushed the savil/config-pkg-to-devpkg branch from 987c28c to cacc711 Compare August 10, 2023 00:58

savil mentioned this pull request Aug 10, 2023

devbox.findPackagesByName should be constructed from Config packages #1360

Merged

savil changed the base branch from savil/config-pkg-to-devpkg to savil/findPackagesByName August 10, 2023 00:58

savil force-pushed the savil/config-packages-2 branch from 94a01fa to 8045218 Compare August 10, 2023 00:58

savil force-pushed the savil/findPackagesByName branch from 819e76f to 63a4ad1 Compare August 10, 2023 01:03

savil force-pushed the savil/config-packages-2 branch from 8045218 to 58a9902 Compare August 10, 2023 01:03

savil requested review from gcurtis and mikeland73 August 10, 2023 01:05

savil marked this pull request as ready for review August 10, 2023 01:05

savil force-pushed the savil/config-packages-2 branch from 58a9902 to ee51c13 Compare August 10, 2023 03:12

savil marked this pull request as draft August 10, 2023 14:36

savil marked this pull request as ready for review August 10, 2023 18:25

savil force-pushed the savil/config-packages-2 branch from 3eb9a45 to 81606cd Compare August 10, 2023 20:59

savil commented Aug 10, 2023

View reviewed changes

savil changed the title ~~[Config Packages] Add platforms and excluded_platforms functionality~~ [per-OS Packages] Add platforms and excluded_platforms functionality Aug 11, 2023

mikeland73 approved these changes Aug 11, 2023

View reviewed changes

savil force-pushed the savil/findPackagesByName branch from 7442274 to 6e0b77c Compare August 11, 2023 22:18

savil force-pushed the savil/config-packages-2 branch from 81606cd to 0dbee1d Compare August 11, 2023 22:18

savil force-pushed the savil/findPackagesByName branch from 6e0b77c to ff1c36e Compare August 11, 2023 22:36

savil force-pushed the savil/config-packages-2 branch from 0dbee1d to 25122b6 Compare August 11, 2023 22:36

savil force-pushed the savil/findPackagesByName branch from ff1c36e to 7e4cebf Compare August 11, 2023 22:46

savil force-pushed the savil/config-packages-2 branch from 25122b6 to 7f2bc7a Compare August 11, 2023 22:46

savil force-pushed the savil/findPackagesByName branch from 7e4cebf to ea87c15 Compare August 11, 2023 23:05

savil force-pushed the savil/config-packages-2 branch from 7f2bc7a to fbba263 Compare August 11, 2023 23:05

Base automatically changed from savil/findPackagesByName to main August 11, 2023 23:06

savil added 5 commits August 11, 2023 16:09

[Config Packages] Add platforms and excluded_platforms functionality

8344529

add VersionedNamesForPlatform and PackageNamesForPlatform

5b8dffb

add test cases for local+remote flakes, and nixpkgs references

fa322ca

add testscript unit test

ddbced8

Ensure we resolve even uninstallable packages in lockfile; undo packa…

8bf71ad

…ge.IsLegacy change

savil force-pushed the savil/config-packages-2 branch from fbba263 to 8bf71ad Compare August 11, 2023 23:09

savil added 2 commits August 11, 2023 16:15

minor requested changes

3cefea3

do best-effort to have Nix system initialized (needed for tests or ot…

35c7cf2

…her direct devbox library callers)

savil merged commit 7a3ae79 into main Aug 12, 2023

savil deleted the savil/config-packages-2 branch August 12, 2023 00:28

[per-OS Packages] Add platforms and excluded_platforms functionality #1358

[per-OS Packages] Add platforms and excluded_platforms functionality #1358

Uh oh!

Conversation

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

Uh oh!

Summary

How was it tested?

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

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!

savil commented Aug 12, 2023

Uh oh!

sentry-io bot commented Aug 14, 2023

Suspect Issues

Uh oh!

Uh oh!

savil commented Aug 9, 2023 •

edited

Loading

savil commented Aug 9, 2023 •

edited

Loading

savil commented Aug 10, 2023 •

edited

Loading