-
Notifications
You must be signed in to change notification settings - Fork 248
[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
Changes from all commits
85b2c95
a735e64
aeaa38e
e00b5c6
bfd8e59
39d01b3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
{ | ||
"packages": [ | ||
"[email protected]", | ||
"[email protected]", | ||
"[email protected]" | ||
], | ||
"env": { | ||
|
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", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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" | ||
|
@@ -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() { | ||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
} | ||
|
||
|
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 | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
package nix | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
) | ||
|
||
func TestContentAddressedPath(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
} | ||
}) | ||
|
||
} | ||
} |
There was a problem hiding this comment.
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:
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)There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 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 thePackage.Installable
here.@gcurtis would you have an opinion?