[Packages] Introduce the concept of Installable packages #1357

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

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:

Replace devbox.Packages with ConfigPackageNames() and InstallablePackageNames().
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.

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

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.

mikeland73

I have 2 main concerns:

Changing the signature of packages function to return an error causes significant code churn and can be avoided by pre-computing the system earlier in the call.
Having 2 similar functions (installable vs config) increases cognitive load on future work. A partial solution is (a) Remove installablePackageNames because I think it is not needed. (b) Make configPackages private.

I think longer term we could also get rid of ConfigPackageNames and just end up with:

InstallablePackages (public)
configPackages (private)

mikeland73 · 2023-08-11T19:16:34Z

internal/devconfig/packages.go

+	return name
+}
+
+func (p *Package) IsEnabledOnPlatform() (bool, error) {


Making this return (bool, error) causes large changes in the code base (and generally makes things bit more difficult to read).

If seems like in the follow up PR you implement this using nix.System

Suggestion:

Have nix.EnsureNixInstalled call nix.ComputeSystem which stores the system in a package variable.

nix.System returns it (and panics if not set).

I know this has a bunch of stuff stacked on it so I don't know how easy it would be to make this change, but it would prevent a good amount of code/blame churn.

I like this; done

I added nix.MustGetSystem, since nix.System currently has an error return refactoring which would introduce a lot of unrelated changes. Can cleanup in a followup PR.

internal/impl/devbox.go

mikeland73 · 2023-08-11T19:25:47Z

internal/plugin/plugin.go

@@ -157,13 +157,18 @@ func (m *Manager) createFile(
 		urlForInput = pkg.URLForFlakeInput()
 	}

+	packages, err := m.InstallablePackageNames()


I think it's fine for this to be all package names. Specifically, this is used so that plugin templates know all packages, it is currently used by php and haskell plugins.

If anything, in a follow up we could change this to be Package structs instead of strings. (and it would be backward compatible because the struct has a String() method.

This would allow us to remove InstallablePackageNames and just keep a single PackageNames.

If anything, in a follow up we could change this to be Package structs instead of strings. (and it would be backward compatible because the struct has a String() method.

Before we rely on .String(), I'd prefer making Package structs not embed url.URL, so we can properly implement String() to do what is intended.

internal/impl/devbox.go

…s; and rename ConfigPackageNames to PackageNames; drop error return from ConfigPackages

sentry-io · 2023-08-15T11:47:17Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

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

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

…nction API (#1374) ## Summary `nix.System` is widely used. So, in #1357 we wanted to change its API to just return `string` instead of `string, error`. To do so, we cache the result in a `nix` package variable. And populate it during `EnsureNixInstalled`. However, `EnsureNixInstalled` was only called from Cobra command functions. But `devbox` as a library also now needs to call it. IMO, it should always have called it since we do rely on `nix` being installed. This PR adds this. It also fixes up the API to: `System: string` and `ComputeSystem: (string, error)`. NOTE: inside `System`, I still do a best-effort to avoid panic by calling `ComputeSystem`. This can happen in edge-cases like some tests `generate_test/TestWriteFromTemplate` that exercise internal functions reliant on `nix.System` without calling `devbox.Open`. ## How was it tested? tests should pass

savil mentioned this pull request Aug 9, 2023

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

Merged

2 tasks

savil changed the title ~~[Packages] Introduce ConfigPackages and PlatformPackage functions~~ [Packages] Introduce ConfigPackages and InstallablePackages functions Aug 9, 2023

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

savil changed the title ~~[Packages] Introduce ConfigPackages and InstallablePackages functions~~ Split devbox.Packages() into ConfigPackages() and InstallablePackages() methods Aug 9, 2023

savil changed the title ~~Split devbox.Packages() into ConfigPackages() and InstallablePackages() methods~~ [Packages] Introduce the concept of Installable packages Aug 10, 2023

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 force-pushed the savil/config-pkg-to-devpkg branch from cacc711 to 9a3354a Compare August 10, 2023 01:03

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

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

mikeland73 approved these changes Aug 11, 2023

View reviewed changes

savil added 5 commits August 11, 2023 15:25

Replace devbox.Packages with ConfigPackageNames and PlatformPackageNames

8f0918c

Split PackagesAsInputs into ConfigPackages and PlatformPackages

a1b5621

rename PackagePackages to InstallablePackages

0cd3329

generate devpkg.Package from Config

86a3232

Cache Nix system in EnsureNixInstalled; Remove InstallablePackageName…

3ba264f

…s; and rename ConfigPackageNames to PackageNames; drop error return from ConfigPackages

savil force-pushed the savil/config-pkg-to-devpkg branch from 9bbc3dc to 3ba264f Compare August 11, 2023 22:36

rename AllPackages to AllInstallablePackage

292803a

savil merged commit 89ef96b into main Aug 11, 2023

savil deleted the savil/config-pkg-to-devpkg branch August 11, 2023 23:05

savil mentioned this pull request Aug 14, 2023

[nix.System] call EnsureNixInstalled from devbox.Open, and cleanup function API #1374

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Packages] Introduce the concept of Installable packages #1357

[Packages] Introduce the concept of Installable packages #1357

Uh oh!

savil commented Aug 9, 2023 •

edited

Loading

Uh oh!

savil commented Aug 9, 2023 •

edited

Loading

Uh oh!

mikeland73 left a comment

Uh oh!

mikeland73 Aug 11, 2023

Uh oh!

savil Aug 11, 2023

Uh oh!

savil Aug 11, 2023

Uh oh!

Uh oh!

mikeland73 Aug 11, 2023

Uh oh!

savil Aug 11, 2023

Uh oh!

savil Aug 11, 2023

Uh oh!

Uh oh!

sentry-io bot commented Aug 15, 2023

Uh oh!

Uh oh!

[Packages] Introduce the concept of Installable packages #1357

[Packages] Introduce the concept of Installable packages #1357

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!

mikeland73 left a comment

Choose a reason for hiding this comment

Uh oh!

mikeland73 Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

savil Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

savil Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mikeland73 Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

savil Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

savil Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sentry-io bot commented Aug 15, 2023

Suspect Issues

Uh oh!

Uh oh!

savil commented Aug 9, 2023 •

edited

Loading

savil commented Aug 9, 2023 •

edited

Loading