Skip to content

[remove nixpkgs] add toPath, and edit devbox.lock only on update command #1256

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 6 commits into from
Jul 7, 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
1 change: 0 additions & 1 deletion devbox.json
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
{
"packages": [
"[email protected]",
"[email protected]",
"[email protected]"
],
"env": {
Expand Down
5 changes: 0 additions & 5 deletions devbox.lock
Original file line number Diff line number Diff line change
@@ -1,11 +1,6 @@
{
"lockfile_version": "1",
"packages": {
"[email protected]": {
"last_modified": "2023-03-31T22:52:29Z",
"resolved": "github:NixOS/nixpkgs/242246ee1e58f54d2322227fc5eef53b4a616a31#actionlint",
"version": "1.6.23"
},
"[email protected]": {
"last_modified": "2023-05-25T03:54:59Z",
"resolved": "github:NixOS/nixpkgs/8d4d822bc0efa9de6eddc79cb0d82897a9baa750#go",
Expand Down
48 changes: 42 additions & 6 deletions internal/devpkg/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"io"
"net/url"
"os"
"path/filepath"
"regexp"
"strings"
Expand All @@ -19,6 +20,7 @@ import (
"go.jetpack.io/devbox/internal/cuecfg"
"go.jetpack.io/devbox/internal/lock"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/ux"
)

// Package represents a "package" added to the devbox.json config.
Expand Down Expand Up @@ -405,8 +407,10 @@ func (p *Package) HashFromNixPkgsURL() string {

// BinaryCacheStore is the store from which to fetch this package's binaries.
// It is used as FromStore in builtins.fetchClosure.
// TODO savil: rename to BinaryCache
const BinaryCacheStore = "https://cache.nixos.org"

// TODO savil: rename to IsInBinaryCache
func (p *Package) IsInBinaryStore() (bool, error) {
if !p.isVersioned() {
return false, nil
Expand All @@ -432,7 +436,8 @@ func (p *Package) IsInBinaryStore() (bool, error) {
}

// PathInBinaryStore is the key in the BinaryCacheStore for this package
// This is used as FromPath in builtins.fetchClosure
// This is used as StorePath in builtins.fetchClosure
// TODO savil: rename to PathInBinaryCache
func (p *Package) PathInBinaryStore() (string, error) {
if isInStore, err := p.IsInBinaryStore(); err != nil {
return "", err
Expand All @@ -452,10 +457,41 @@ func (p *Package) PathInBinaryStore() (string, error) {
}

sysInfo := entry.Systems[userSystem]
storeDirParts := []string{sysInfo.FromHash, sysInfo.StoreName}
if sysInfo.StoreVersion != "" {
storeDirParts = append(storeDirParts, sysInfo.StoreVersion)
return sysInfo.StorePath, nil
}

func (p *Package) ContentAddressedStorePath() (string, error) {

if isInStore, err := p.IsInBinaryStore(); err != nil {
return "", err
} else if !isInStore {
return "",
errors.Errorf("Package %q cannot be fetched from binary cache store", p.Raw)
}

entry, err := p.lockfile.Resolve(p.Raw)
if err != nil {
return "", err
}

userSystem, err := nix.System()
if err != nil {
return "", err
}

sysInfo := entry.Systems[userSystem]
if sysInfo.CAStorePath != "" {
return sysInfo.CAStorePath, nil
}

ux.Fwarning(
os.Stderr,
"calculating local_store_path. This may be slow.\n"+
"Run `devbox update` to speed this up for next time.",
)
localPath, err := nix.ContentAddressedStorePath(sysInfo.StorePath)
if err != nil {
return "", err
}
storeDir := strings.Join(storeDirParts, "-")
return filepath.Join("/nix/store", storeDir), nil
return localPath, err
}
42 changes: 36 additions & 6 deletions internal/impl/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"fmt"

"go.jetpack.io/devbox/internal/boxcli/featureflag"
"go.jetpack.io/devbox/internal/devpkg"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/nix/nixprofile"
Expand Down Expand Up @@ -90,21 +91,50 @@ func (d *Devbox) updateDevboxPackage(
if err != nil {
return err
}
if existing != nil && existing.Version != newEntry.Version {
if existing == nil {
ux.Finfo(d.writer, "Resolved %s to %[1]s %[2]s\n", pkg, newEntry.Resolved)
d.lockfile.Packages[pkg.Raw] = newEntry
return nil
}

if existing.Version != newEntry.Version {
ux.Finfo(d.writer, "Updating %s %s -> %s\n", pkg, existing.Version, newEntry.Version)
if err := d.removePackagesFromProfile(ctx, []string{pkg.Raw}); err != nil {
// Warn but continue. TODO(landau): ensurePackagesAreInstalled should
// sync the profile so we don't need to do this manually.
ux.Fwarning(d.writer, "Failed to remove %s from profile: %s\n", pkg, err)
}
d.lockfile.Packages[pkg.Raw] = newEntry
} else if existing == nil {
ux.Finfo(d.writer, "Resolved %s to %[1]s %[2]s\n", pkg, newEntry.Resolved)
d.lockfile.Packages[pkg.Raw] = newEntry
} else {
ux.Finfo(d.writer, "Already up-to-date %s %s\n", pkg, existing.Version)
return nil
}

// Check if the package's system info is missing, or not complete.
if featureflag.RemoveNixpkgs.Enabled() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of the logic in this block could be encapsulated in lock file. Maybe:

if existing.CanBeUpdated(newEntry) {
  d.lockfile.Update(existing, newEntry)
}

or something like that. It could deal with version changes and also missing system stuff.

That way we could make Package.Systems semi-private. (It still needs to be exported because it is part of the JSON)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, is this logic block missing profile install/uninstall? e.g. if a package previously required nixpkgs and we replace it with one that does not require it, do we want to update the profile?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, I agree this could likely be moved into the lock package. In general, I think quite a bit of the update code (beyond this block) could be better encapsulated. I may not have time to get to it though...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also, is this logic block missing profile install/uninstall? e.g. if a package previously required nixpkgs and we replace it with one that does not require it, do we want to update the profile?

🤔 yeah, this one I am unsure about. In theory, the package's binary should be the exact same.

Its also why I think it was okay and/or put a question mark after using InputAddressedPath in the Package.Installable here.

@gcurtis would you have an opinion?

userSystem, err := nix.System()
if err != nil {
return err
}

// Check if the system info is missing for the user's system.
sysInfo := d.lockfile.Packages[pkg.Raw].Systems[userSystem]
if sysInfo == nil {
d.lockfile.Packages[pkg.Raw] = newEntry
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, could this override an existing (different) system that already has a CA path?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are right! Much obliged 🙏🏾

sending fix: #1259

ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
return nil
}

// Check if the CAStorePath is missing for the user's system.
// Since any one user cannot add this field for all systems,
// we'll need to progressively add it to a project's lockfile.
if sysInfo.CAStorePath == "" {
// Update the CAStorePath for the user's system
d.lockfile.Packages[pkg.Raw].Systems[userSystem].CAStorePath = newEntry.Systems[userSystem].CAStorePath
ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
return nil
}
}

ux.Finfo(d.writer, "Already up-to-date %s %s\n", pkg, existing.Version)
return nil
}

Expand Down
40 changes: 8 additions & 32 deletions internal/lock/lockfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ import (

"github.com/pkg/errors"
"github.com/samber/lo"
"go.jetpack.io/devbox/internal/boxcli/featureflag"
"go.jetpack.io/devbox/internal/nix"
"go.jetpack.io/devbox/internal/searcher"

"go.jetpack.io/devbox/internal/cuecfg"
Expand Down Expand Up @@ -40,12 +38,13 @@ type Package struct {
}

type SystemInfo struct {
System string // stored elsewhere in json: it's the key for the Package.Systems
FromHash string `json:"from_hash,omitempty"`
// StoreName may be different from the canonicalName so we store it separately
StoreName string `json:"store_name,omitempty"`
StoreVersion string `json:"store_version,omitempty"`
ToHash string `json:"to_hash,omitempty"`
// StorePath is the cache key in the Binary Cache Store (cache.nixos.org)
// It is of the form <hash>-<name>-<version>
// <name> may be different from the canonicalName so we store the full store path.
StorePath string `json:"store_path,omitempty"`
// CAStorePath is the content-addressed path for the nix package in /nix/store
// It is of the form <hash>-<name>-<version>
CAStorePath string `json:"ca_store_path,omitempty"`
}

func GetFile(project devboxProject) (*File, error) {
Expand Down Expand Up @@ -86,17 +85,7 @@ func (l *File) Remove(pkgs ...string) error {
func (l *File) Resolve(pkg string) (*Package, error) {
entry, hasEntry := l.Packages[pkg]

// If the package's system info is missing, we need to resolve it again.
needsSysInfo := false
if hasEntry && featureflag.RemoveNixpkgs.Enabled() {
userSystem, err := nix.System()
if err != nil {
return nil, err
}
needsSysInfo = entry.Systems[userSystem] == nil
}

if !hasEntry || entry.Resolved == "" || needsSysInfo {
if !hasEntry || entry.Resolved == "" {
locked := &Package{}
var err error
if _, _, versioned := searcher.ParseVersionedPackage(pkg); versioned {
Expand All @@ -109,19 +98,6 @@ func (l *File) Resolve(pkg string) (*Package, error) {
// whatever hash is in the devbox.json
locked = &Package{Resolved: l.LegacyNixpkgsPath(pkg)}
}

// Merge the system info from the lockfile with the queried system info.
// This is necessary because a different system's info may previously have
// been added. For example: `aarch64-darwin` was already added, but
// current user is on `x86_64-linux`.
if hasEntry && featureflag.RemoveNixpkgs.Enabled() {
for _, sysInfo := range entry.Systems {
if _, ok := locked.Systems[sysInfo.System]; !ok {
locked.Systems[sysInfo.System] = sysInfo
}
}
}

l.Packages[pkg] = locked
}

Expand Down
34 changes: 27 additions & 7 deletions internal/lock/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ func (l *File) FetchResolvedPackage(pkg string) (*Package, error) {

sysInfos := map[string]*SystemInfo{}
if featureflag.RemoveNixpkgs.Enabled() {
sysInfos = buildLockSystemInfos(packageVersion)
sysInfos, err = buildLockSystemInfos(packageVersion)
if err != nil {
return nil, err
}
}
packageInfo, err := selectForSystem(packageVersion)
if err != nil {
Expand Down Expand Up @@ -76,15 +79,32 @@ func selectForSystem(pkg *searcher.PackageVersion) (searcher.PackageInfo, error)
return maps.Values(pkg.Systems)[0], nil
}

func buildLockSystemInfos(pkg *searcher.PackageVersion) map[string]*SystemInfo {
func buildLockSystemInfos(pkg *searcher.PackageVersion) (map[string]*SystemInfo, error) {
userSystem, err := nix.System()
if err != nil {
return nil, err
}

sysInfos := map[string]*SystemInfo{}
for sysName, sysInfo := range pkg.Systems {

// guard against missing search data
if sysInfo.StoreHash == "" || sysInfo.StoreName == "" {
continue
}

storePath := nix.StorePath(sysInfo.StoreHash, sysInfo.StoreName, sysInfo.StoreVersion)
caStorePath := ""
if sysName == userSystem {
caStorePath, err = nix.ContentAddressedStorePath(storePath)
if err != nil {
return nil, err
}
}
sysInfos[sysName] = &SystemInfo{
System: sysName,
FromHash: sysInfo.StoreHash,
StoreName: sysInfo.StoreName,
StoreVersion: sysInfo.StoreVersion,
StorePath: storePath,
CAStorePath: caStorePath,
}
}
return sysInfos
return sysInfos, nil
}
3 changes: 0 additions & 3 deletions internal/nix/nix.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,6 @@ func (*Nix) PrintDevEnv(ctx context.Context, args *PrintDevEnvArgs) (*PrintDevEn
args.FlakesFilePath,
)
cmd.Args = append(cmd.Args, ExperimentalFlags()...)
if featureflag.RemoveNixpkgs.Enabled() {
cmd.Args = append(cmd.Args, "--impure")
}
cmd.Args = append(cmd.Args, "--json")
debug.Log("Running print-dev-env cmd: %s\n", cmd)
data, err = cmd.Output()
Expand Down
44 changes: 44 additions & 0 deletions internal/nix/store.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package nix

import (
"encoding/json"
"path/filepath"
"strings"

"github.com/pkg/errors"
)

func StorePath(hash, name, version string) string {
storeDirParts := []string{hash, name}
if version != "" {
storeDirParts = append(storeDirParts, version)
}
storeDir := strings.Join(storeDirParts, "-")
return filepath.Join("/nix/store", storeDir)
}

// ContentAddressedStorePath takes a store path and returns the content-addressed store path.
func ContentAddressedStorePath(storePath string) (string, error) {
cmd := command("store", "make-content-addressed", storePath, "--json")
out, err := cmd.Output()
if err != nil {
return "", errors.WithStack(err)
}
// Example Output:
// > nix store make-content-addressed /nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1 --json
// {"rewrites":{"/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1":"/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1"}}

type ContentAddressed struct {
Rewrites map[string]string `json:"rewrites"`
}
caOutput := ContentAddressed{}
if err := json.Unmarshal(out, &caOutput); err != nil {
return "", errors.WithStack(err)
}

caStorePath, ok := caOutput.Rewrites[storePath]
if !ok {
return "", errors.Errorf("could not find content-addressed store path for %s", storePath)
}
return caStorePath, nil
}
32 changes: 32 additions & 0 deletions internal/nix/store_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package nix

import (
"fmt"
"testing"
)

func TestContentAddressedPath(t *testing.T) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍🏻


testCases := []struct {
storePath string
expected string
}{
{
"/nix/store/r2jd6ygnmirm2g803mksqqjm4y39yi6i-git-2.33.1",
"/nix/store/ldbhlwhh39wha58rm61bkiiwm6j7211j-git-2.33.1",
Comment on lines +15 to +16
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does Nix automatically download these if they're not in the store yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep

},
}

for index, testCase := range testCases {
t.Run(fmt.Sprintf("%d", index), func(t *testing.T) {
out, err := ContentAddressedStorePath(testCase.storePath)
if err != nil {
t.Errorf("got error: %v", err)
}
if out != testCase.expected {
t.Errorf("got %s, want %s", out, testCase.expected)
}
})

}
}
1 change: 1 addition & 0 deletions internal/shellgen/tmpl/flake_remove_nixpkgs.nix.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
(builtins.fetchClosure{
fromStore = "{{ $.BinaryCacheStore }}";
fromPath = "{{ .PathInBinaryStore }}";
toPath = "{{ .ContentAddressedStorePath }}";
})
{{- end }}
{{- end }}
Expand Down