Skip to content

Commit

Permalink
devbox add fallthrough and fallback for packages not handled by search (
Browse files Browse the repository at this point in the history
#1264)

## Summary
`devbox add` fallthrough and fallback for packages not handled by
search.

This PR does not do the following:
1. Print a warning that a package is in a fallthrough case (I'm unclear
if a warning is needed?)
2. Accept an alternative nixpkg commit hash for that specific package
(I'm unclear if we are deprecating `nixpkg` commit field in
`devbox.json` immediately?)

I'd like to address those two things above if needed in a separate PR as
a follow up.

Example lockfile:
```
{
  "lockfile_version": "1",
  "packages": {
    "python310": {
      "plugin_version": "0.0.1",
      "resolved": "github:NixOS/nixpkgs/f80ac848e3d6f0c12c52758c0f25c10c97ca3b62#python310",
      "source": "nixpkg"
    },
    "python310Packages.pip@latest": {
      "last_modified": "2023-05-06T16:57:53Z",
      "plugin_version": "0.0.1",
      "resolved": "github:NixOS/nixpkgs/16b3b0c53b1ee8936739f8c588544e7fcec3fc60#python310Packages.pip",
      "source": "devbox-search",
      "version": "23.0.1"
    },
    "stdenv.cc.cc.lib": {
      "resolved": "github:NixOS/nixpkgs/f80ac848e3d6f0c12c52758c0f25c10c97ca3b62#stdenv.cc.cc.lib",
      "source": "nixpkg"
    }
  }
}

```

## How was it tested?
In the Tensorflow example:
```
devbox add stdenv.cc.cc.lib
devbox add nodejs-16_x
devbox update
```
  • Loading branch information
LucilleH authored Jul 11, 2023
1 parent dea479d commit f6f94fe
Show file tree
Hide file tree
Showing 7 changed files with 38 additions and 34 deletions.
2 changes: 1 addition & 1 deletion internal/devpkg/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ func (p *Package) Versioned() string {
}

func (p *Package) IsLegacy() bool {
return p.isDevboxPackage() && !p.isVersioned()
return p.isDevboxPackage() && !p.isVersioned() && p.lockfile.Source(p.Raw) == ""
}

func (p *Package) LegacyToVersioned() string {
Expand Down
4 changes: 4 additions & 0 deletions internal/devpkg/package_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,10 @@ func (l *lockfile) LegacyNixpkgsPath(pkg string) string {
)
}

func (l *lockfile) Source(pkg string) string {
return ""
}

func (l *lockfile) Resolve(pkg string) (*lock.Package, error) {
switch {
case strings.Contains(pkg, "path:"):
Expand Down
25 changes: 10 additions & 15 deletions internal/impl/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,8 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
// replace it.
pkgs := []*devpkg.Package{}
for _, pkg := range devpkg.PackageFromStrings(lo.Uniq(pkgsNames), d.lockfile) {
versioned := pkg.Versioned()

// If exact versioned package is already in the config, skip.
if slices.Contains(d.cfg.Packages, versioned) {
if slices.Contains(d.cfg.Packages, pkg.Versioned()) {
continue
}

Expand All @@ -59,18 +57,15 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
}
}

pkgs = append(pkgs, devpkg.PackageFromString(versioned, d.lockfile))

This comment has been minimized.

Copy link
@mikeland73

mikeland73 Jul 11, 2023

Contributor

Removing this line causes pkgs to be empty later on. We are no longer explicitly adding to lockfile (though because of some other code the packages get added anyway). Another issue is we don't end up showing the readme

d.cfg.Packages = append(d.cfg.Packages, versioned)
}

// Check packages are valid before adding.
for _, pkg := range pkgs {
ok, err := pkg.ValidateExists()
if err != nil {
return err
}
if !ok {
return errors.Wrap(nix.ErrPackageNotFound, pkg.Raw)
// validate that the versioned package exists in the search endpoint.
// if not, fallback to legacy vanilla nix.
versionedPkg := devpkg.PackageFromString(pkg.Versioned(), d.lockfile)
ok, err := versionedPkg.ValidateExists()
if err == nil && ok {
d.cfg.Packages = append(d.cfg.Packages, pkg.Versioned())
} else {
// fallthrough and treat package as a legacy package.
d.cfg.Packages = append(d.cfg.Packages, pkg.Raw)

This comment has been minimized.

Copy link
@mikeland73

mikeland73 Jul 11, 2023

Contributor

This can lead to not so great error messages. As a follow up may make sense to use nix.Search to check if package exists

image
}
}

Expand Down
10 changes: 4 additions & 6 deletions internal/impl/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,10 @@ func (d *Devbox) Update(ctx context.Context, pkgs ...string) error {
if err := d.Remove(ctx, pkg.Raw); err != nil {
return err
}
if err := d.lockfile.ResolveToCurrentNixpkgCommitHash(
pkg.LegacyToVersioned(),
); err != nil {
return err
}
if err := d.Add(ctx, pkg.LegacyToVersioned()); err != nil {
// 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, pkg.Raw); err != nil {
return err
}
} else {
Expand Down
1 change: 1 addition & 0 deletions internal/lock/interfaces.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,5 @@ type Locker interface {
LegacyNixpkgsPath(string) string
ProjectDir() string
Resolve(string) (*Package, error)
Source(string) string
}
29 changes: 17 additions & 12 deletions internal/lock/lockfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ import (
)

const lockFileVersion = "1"
const (
nixpkgSource string = "nixpkg"
devboxSearchSource string = "devbox-search"
)

// Lightly inspired by package-lock.json
type File struct {
Expand All @@ -32,6 +36,7 @@ type Package struct {
LastModified string `json:"last_modified,omitempty"`
PluginVersion string `json:"plugin_version,omitempty"`
Resolved string `json:"resolved,omitempty"`
Source string `json:"source,omitempty"`
Version string `json:"version,omitempty"`
// Systems is keyed by the system name
Systems map[string]*SystemInfo `json:"systems,omitempty"`
Expand Down Expand Up @@ -97,7 +102,10 @@ func (l *File) Resolve(pkg string) (*Package, error) {
} else if IsLegacyPackage(pkg) {
// These are legacy packages without a version. Resolve to nixpkgs with
// whatever hash is in the devbox.json
locked = &Package{Resolved: l.LegacyNixpkgsPath(pkg)}
locked = &Package{
Resolved: l.LegacyNixpkgsPath(pkg),
Source: nixpkgSource,
}
}
l.Packages[pkg] = locked
}
Expand All @@ -110,17 +118,6 @@ func (l *File) ForceResolve(pkg string) (*Package, error) {
return l.Resolve(pkg)
}

func (l *File) ResolveToCurrentNixpkgCommitHash(pkg string) error {
name, version, found := strings.Cut(pkg, "@")
if found && version != "latest" {
return errors.New(
"only allowed version is @latest. Otherwise we can't guarantee the " +
"version will resolve")
}
l.Packages[pkg] = &Package{Resolved: l.LegacyNixpkgsPath(name)}
return nil
}

func (l *File) Save() error {
return cuecfg.WriteFile(lockFilePath(l.devboxProject), l)
}
Expand All @@ -133,6 +130,14 @@ func (l *File) LegacyNixpkgsPath(pkg string) string {
)
}

func (l *File) Source(pkg string) string {
entry, hasEntry := l.Packages[pkg]
if !hasEntry || entry.Resolved == "" {
return ""
}
return entry.Source
}

// This probably belongs in input.go but can't add it there because it will
// create a circular dependency. We could move Input into own package.
func IsLegacyPackage(pkg string) bool {
Expand Down
1 change: 1 addition & 0 deletions internal/lock/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ func (l *File) FetchResolvedPackage(pkg string) (*Package, error) {
packageInfo.AttrPaths[0],
),
Version: packageInfo.Version,
Source: devboxSearchSource,
Systems: sysInfos,
}, nil
}
Expand Down

0 comments on commit f6f94fe

Please sign in to comment.