Skip to content

Commit 6807da6

Browse files
authored
[rm-nixpkgs] Fix profile list bug for packages added by store path (#1416)
## Summary When a package is added by store path, then when we query `nix profile list`, it'll look like this: ``` > nix profile list --profile .devbox/nix/profile/default Index: 0 Store paths: /nix/store/yjkac3v6c1v129fph0gvy9c7ndnx738b-cowsay-3.04 ``` instead of this: ``` > nix profile list --profile .devbox/nix/profile/default Index: 0 Flake attribute: legacyPackages.x86_64-darwin.hello Original flake URL: github:NixOS/nixpkgs/3007746b3f5bfcb49e102b517bca891822a41b31 Locked flake URL: github:NixOS/nixpkgs/3007746b3f5bfcb49e102b517bca891822a41b31 Store paths: /nix/store/gyh1bri77vlcs1pwxyg4x3z06aql2c6p-hello-2.12.1 ``` Thus, when we read the profile items and try to build a map out of the item's unlockedRef, we were always ending up with a map of size 1. To fix, I changed the map to a slice, and changed the callsites to handle the slice instead. I made a few other code organization changes that are no-ops. ## How was it tested? Tested by calling `devbox install` and inspecting the profile with `nix profile list`, with and without the rm-nixpkgs feature enabled. I attempted to add unit tests but it's not as easy as I had thought.
1 parent 9aa4750 commit 6807da6

File tree

7 files changed

+149
-118
lines changed

7 files changed

+149
-118
lines changed

.github/workflows/cli-tests.yaml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ jobs:
134134
DEVBOX_DEBUG: ${{ (!matrix.run-devbox-json-tests || inputs.example-debug) && '1' || '0' }}
135135
DEVBOX_RUN_DEVBOX_JSON_TESTS: ${{ matrix.run-devbox-json-tests }}
136136
# Used in `go test -timeout` flag. Needs a value that time.ParseDuration can parse.
137-
DEVBOX_GOLANG_TEST_TIMEOUT: "${{ (github.ref == 'refs/heads/main' || inputs.run-mac-tests) && '35m' || '25m' }}"
137+
DEVBOX_GOLANG_TEST_TIMEOUT: "${{ (github.ref == 'refs/heads/main' || inputs.run-mac-tests) && '35m' || '30m' }}"
138138
run: |
139139
echo "::group::Nix version"
140140
nix --version

internal/devpkg/package.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -372,6 +372,9 @@ func (p *Package) Hash() string {
372372
return shortHash
373373
}
374374

375+
// Equals compares two Packages. This may be an expensive operation since it
376+
// may have to normalize a Package's attribute path, which may require a network
377+
// call.
375378
func (p *Package) Equals(other *Package) bool {
376379
if p.String() == other.String() {
377380
return true

internal/impl/devbox.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -974,7 +974,7 @@ func (d *Devbox) InstallablePackages() []*devpkg.Package {
974974
})
975975
}
976976

977-
// InstallableAndPluginPackages returns installable user packages and plugin
977+
// AllInstallablePackages returns installable user packages and plugin
978978
// packages concatenated in correct order
979979
func (d *Devbox) AllInstallablePackages() ([]*devpkg.Package, error) {
980980
userPackages := d.InstallablePackages()

internal/impl/packages.go

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -352,18 +352,18 @@ func (d *Devbox) removePackagesFromProfile(ctx context.Context, pkgs []string) e
352352
return err
353353
}
354354

355-
for _, input := range packages {
355+
for _, pkg := range packages {
356356
index, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{
357357
Lockfile: d.lockfile,
358358
Writer: d.writer,
359-
Input: input,
359+
Package: pkg,
360360
ProfileDir: profileDir,
361361
})
362362
if err != nil {
363363
ux.Ferror(
364364
d.writer,
365365
"Package %s not found in profile. Skipping.\n",
366-
input.Raw,
366+
pkg.Raw,
367367
)
368368
continue
369369
}
@@ -415,7 +415,7 @@ func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]*devpkg.
415415
}
416416

417417
pending := []*devpkg.Package{}
418-
list, err := nixprofile.ProfileListItems(d.writer, profileDir)
418+
items, err := nixprofile.ProfileListItems(d.writer, profileDir)
419419
if err != nil {
420420
return nil, err
421421
}
@@ -428,10 +428,10 @@ func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]*devpkg.
428428
}
429429
for _, pkg := range packages {
430430
_, err := nixprofile.ProfileListIndex(&nixprofile.ProfileListIndexArgs{
431-
List: list,
431+
Items: items,
432432
Lockfile: d.lockfile,
433433
Writer: d.writer,
434-
Input: pkg,
434+
Package: pkg,
435435
ProfileDir: profileDir,
436436
})
437437
if err != nil {
@@ -444,7 +444,7 @@ func (d *Devbox) pendingPackagesForInstallation(ctx context.Context) ([]*devpkg.
444444
return pending, nil
445445
}
446446

447-
// extraPkgsInProfile returns a list of packages that are in the nix profile,
447+
// extraPackagesInProfile returns a list of packages that are in the nix profile,
448448
// but are NOT in devbox.json or global devbox.json.
449449
//
450450
// NOTE: as an optimization, this implementation assumes that all packages in

internal/nix/nixprofile/item.go

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
package nixprofile
2+
3+
import (
4+
"fmt"
5+
"strings"
6+
7+
"go.jetpack.io/devbox/internal/devpkg"
8+
"go.jetpack.io/devbox/internal/lock"
9+
"go.jetpack.io/devbox/internal/redact"
10+
)
11+
12+
// NixProfileListItem is a go-struct of a line of printed output from `nix profile list`
13+
// docs: https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile-list.html
14+
type NixProfileListItem struct {
15+
// An integer that can be used to unambiguously identify the package in
16+
// invocations of nix profile remove and nix profile upgrade.
17+
index int
18+
19+
// The original ("unlocked") flake reference and output attribute path used at installation time.
20+
// NOTE that this can be "#" if the package was added to the nix profile via store path.
21+
unlockedReference string
22+
23+
// The locked flake reference to which the unlocked flake reference was resolved.
24+
// NOTE that this can be "#" if the package was added to the nix profile via store path.
25+
lockedReference string
26+
27+
// The store path(s) of the package.
28+
nixStorePath string // TODO: change to slice
29+
}
30+
31+
// AttributePath parses the package attribute from the NixProfileListItem.lockedReference
32+
//
33+
// For example:
34+
// if NixProfileListItem.lockedReference = github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
35+
// then AttributePath = legacyPackages.x86_64-darwin.go_1_19
36+
func (item *NixProfileListItem) AttributePath() (string, error) {
37+
// lockedReference example:
38+
// github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
39+
40+
// AttributePath example:
41+
// legacyPackages.x86_64.go_1_19
42+
_ /*nixpkgs*/, attrPath, found := strings.Cut(item.lockedReference, "#")
43+
if !found {
44+
return "", redact.Errorf(
45+
"expected to find # in lockedReference: %s from NixProfileListItem: %s",
46+
redact.Safe(item.lockedReference),
47+
item,
48+
)
49+
}
50+
return attrPath, nil
51+
}
52+
53+
// ToPackage constructs a nix.Package using the unlocked reference.
54+
// TODO: this doesn't handle well the case where item.unlockedReference == "#"
55+
func (item *NixProfileListItem) ToPackage(locker lock.Locker) *devpkg.Package {
56+
return devpkg.PackageFromString(item.unlockedReference, locker)
57+
}
58+
59+
// String serializes the NixProfileListItem back into the format printed by `nix profile list`
60+
func (item *NixProfileListItem) String() string {
61+
return fmt.Sprintf("%d %s %s %s",
62+
item.index,
63+
item.unlockedReference,
64+
item.lockedReference,
65+
item.nixStorePath,
66+
)
67+
}

internal/nix/nixprofile/profile.go

Lines changed: 69 additions & 108 deletions
Original file line numberDiff line numberDiff line change
@@ -25,45 +25,51 @@ import (
2525
func ProfileListItems(
2626
writer io.Writer,
2727
profileDir string,
28-
) (map[string]*NixProfileListItem, error) {
28+
) ([]*NixProfileListItem, error) {
2929

3030
output, err := nix.ProfileList(writer, profileDir, true /*useJSON*/)
31-
if err == nil {
32-
type ProfileListElement struct {
33-
Active bool `json:"active"`
34-
AttrPath string `json:"attrPath"`
35-
OriginalURL string `json:"originalUrl"`
36-
Priority int `json:"priority"`
37-
StorePaths []string `json:"storePaths"`
38-
URL string `json:"url"`
39-
}
40-
type ProfileListOutput struct {
41-
Elements []ProfileListElement `json:"elements"`
42-
Version int `json:"version"`
43-
}
31+
if err != nil {
32+
// fallback to legacy profile list
33+
// NOTE: maybe we should check the nix version first, instead of falling back on _any_ error.
34+
return profileListLegacy(writer, profileDir)
35+
}
4436

45-
var structOutput ProfileListOutput
46-
if err := json.Unmarshal([]byte(output), &structOutput); err != nil {
47-
return nil, err
48-
}
37+
type ProfileListElement struct {
38+
Active bool `json:"active"`
39+
AttrPath string `json:"attrPath"`
40+
OriginalURL string `json:"originalUrl"`
41+
Priority int `json:"priority"`
42+
StorePaths []string `json:"storePaths"`
43+
URL string `json:"url"`
44+
}
45+
type ProfileListOutput struct {
46+
Elements []ProfileListElement `json:"elements"`
47+
Version int `json:"version"`
48+
}
4949

50-
result := map[string]*NixProfileListItem{}
51-
for index, element := range structOutput.Elements {
52-
// We use the unlocked reference as the key, since that is the format
53-
// used for the `nix profile list` output of older nix versions
54-
// (pre 2.17), which our code is designed to support.
55-
unlockedReference := element.OriginalURL + "#" + element.AttrPath
56-
result[unlockedReference] = &NixProfileListItem{
57-
index: index,
58-
unlockedReference: unlockedReference,
59-
lockedReference: element.URL + "#" + element.AttrPath,
60-
nixStorePath: element.StorePaths[0],
61-
}
62-
}
63-
return result, nil
50+
var structOutput ProfileListOutput
51+
if err := json.Unmarshal([]byte(output), &structOutput); err != nil {
52+
return nil, err
53+
}
54+
55+
items := []*NixProfileListItem{}
56+
for index, element := range structOutput.Elements {
57+
items = append(items, &NixProfileListItem{
58+
index: index,
59+
unlockedReference: element.OriginalURL + "#" + element.AttrPath,
60+
lockedReference: element.URL + "#" + element.AttrPath,
61+
nixStorePath: element.StorePaths[0],
62+
})
6463
}
64+
return items, nil
65+
}
6566

66-
output, err = nix.ProfileList(writer, profileDir, false /*useJSON*/)
67+
// profileListLegacy lists the items in a nix profile before nix 2.17.0 introduced --json.
68+
func profileListLegacy(
69+
writer io.Writer,
70+
profileDir string,
71+
) ([]*NixProfileListItem, error) {
72+
output, err := nix.ProfileList(writer, profileDir, false /*useJSON*/)
6773
if err != nil {
6874
return nil, errors.WithStack(err)
6975
}
@@ -77,7 +83,7 @@ func ProfileListItems(
7783
// 0 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19 /nix/store/w0lyimyyxxfl3gw40n46rpn1yjrl3q85-go-1.19.3
7884
// 1 github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.vim github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.vim /nix/store/gapbqxx1d49077jk8ay38z11wgr12p23-vim-9.0.0609
7985

80-
items := map[string]*NixProfileListItem{}
86+
items := []*NixProfileListItem{}
8187
for _, line := range lines {
8288
line = strings.TrimSpace(line)
8389
if line == "" {
@@ -88,83 +94,75 @@ func ProfileListItems(
8894
return nil, err
8995
}
9096

91-
items[item.unlockedReference] = item
97+
items = append(items, item)
9298
}
9399
return items, nil
94100
}
95101

96102
type ProfileListIndexArgs struct {
97-
// For performance you can reuse the same list in multiple operations if you
103+
// For performance, you can reuse the same list in multiple operations if you
98104
// are confident index has not changed.
99-
List map[string]*NixProfileListItem
105+
Items []*NixProfileListItem
100106
Lockfile *lock.File
101107
Writer io.Writer
102-
Input *devpkg.Package
108+
Package *devpkg.Package
103109
ProfileDir string
104110
}
105111

112+
// ProfileListIndex returns the index of args.Package in the nix profile specified by args.ProfileDir,
113+
// or -1 if it's not found. Callers can pass in args.Items to avoid having to call `nix-profile list` again.
106114
func ProfileListIndex(args *ProfileListIndexArgs) (int, error) {
107115
var err error
108-
list := args.List
109-
if list == nil {
110-
list, err = ProfileListItems(args.Writer, args.ProfileDir)
116+
items := args.Items
117+
if items == nil {
118+
items, err = ProfileListItems(args.Writer, args.ProfileDir)
111119
if err != nil {
112120
return -1, err
113121
}
114122
}
115123

116-
inCache, err := args.Input.IsInBinaryCache()
124+
inCache, err := args.Package.IsInBinaryCache()
117125
if err != nil {
118126
return -1, err
119127
}
120128
if inCache {
121-
pathInStore, err := args.Input.Installable()
129+
// Packages in cache are added by store path, which means we only need to check
130+
// for store path equality to find it.
131+
pathInStore, err := args.Package.Installable()
122132
if err != nil {
123-
return -1, err
133+
return -1, errors.Wrapf(err, "failed to get installable for %s", args.Package.String())
124134
}
125-
for _, item := range list {
135+
for _, item := range items {
126136
if pathInStore == item.nixStorePath {
127137
return item.index, nil
128138
}
129139
}
140+
return -1, errors.Wrap(nix.ErrPackageNotFound, args.Package.String())
130141
}
131-
// else: fallback to checking if the Input matches an item's unlockedReference
132142

133-
// This is an optimization for happy path. A resolved devbox package
134-
// should match the unlockedReference of an existing profile item.
135-
ref, err := args.Input.NormalizedDevboxPackageReference()
143+
// else: check if the Package matches an item's unlockedReference.
144+
// This is an optimization for happy path. A resolved devbox package *which was added by
145+
// flake reference* (not by store path) should match the unlockedReference of an existing
146+
// profile item.
147+
ref, err := args.Package.NormalizedDevboxPackageReference()
136148
if err != nil {
137149
return -1, err
138150
}
139-
if item, found := list[ref]; found {
140-
return item.index, nil
151+
for _, item := range items {
152+
if ref == item.unlockedReference {
153+
return item.index, nil
154+
}
141155
}
142156

143-
for _, item := range list {
157+
// Still not found? Check for full pkg equality (may be expensive).
158+
for _, item := range items {
144159
existing := item.ToPackage(args.Lockfile)
145160

146-
if args.Input.Equals(existing) {
161+
if args.Package.Equals(existing) {
147162
return item.index, nil
148163
}
149164
}
150-
return -1, errors.Wrap(nix.ErrPackageNotFound, args.Input.String())
151-
}
152-
153-
// NixProfileListItem is a go-struct of a line of printed output from `nix profile list`
154-
// docs: https://nixos.org/manual/nix/stable/command-ref/new-cli/nix3-profile-list.html
155-
type NixProfileListItem struct {
156-
// An integer that can be used to unambiguously identify the package in
157-
// invocations of nix profile remove and nix profile upgrade.
158-
index int
159-
160-
// The original ("unlocked") flake reference and output attribute path used at installation time.
161-
unlockedReference string
162-
163-
// The locked flake reference to which the unlocked flake reference was resolved.
164-
lockedReference string
165-
166-
// The store path(s) of the package.
167-
nixStorePath string
165+
return -1, errors.Wrap(nix.ErrPackageNotFound, args.Package.String())
168166
}
169167

170168
// parseNixProfileListItem reads each line of output (from `nix profile list`) and converts
@@ -205,43 +203,6 @@ func parseNixProfileListItem(line string) (*NixProfileListItem, error) {
205203
}, nil
206204
}
207205

208-
// AttributePath parses the package attribute from the NixProfileListItem.lockedReference
209-
//
210-
// For example:
211-
// if NixProfileListItem.lockedReference = github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
212-
// then AttributePath = legacyPackages.x86_64-darwin.go_1_19
213-
func (item *NixProfileListItem) AttributePath() (string, error) {
214-
// lockedReference example:
215-
// github:NixOS/nixpkgs/52e3e80afff4b16ccb7c52e9f0f5220552f03d04#legacyPackages.x86_64-darwin.go_1_19
216-
217-
// AttributePath example:
218-
// legacyPackages.x86_64.go_1_19
219-
_ /*nixpkgs*/, attrPath, found := strings.Cut(item.lockedReference, "#")
220-
if !found {
221-
return "", redact.Errorf(
222-
"expected to find # in lockedReference: %s from NixProfileListItem: %s",
223-
redact.Safe(item.lockedReference),
224-
item,
225-
)
226-
}
227-
return attrPath, nil
228-
}
229-
230-
// ToPackage constructs a nix.Package using the unlocked reference
231-
func (item *NixProfileListItem) ToPackage(locker lock.Locker) *devpkg.Package {
232-
return devpkg.PackageFromString(item.unlockedReference, locker)
233-
}
234-
235-
// String serializes the NixProfileListItem back into the format printed by `nix profile list`
236-
func (item *NixProfileListItem) String() string {
237-
return fmt.Sprintf("%d %s %s %s",
238-
item.index,
239-
item.unlockedReference,
240-
item.lockedReference,
241-
item.nixStorePath,
242-
)
243-
}
244-
245206
type ProfileInstallArgs struct {
246207
CustomStepMessage string
247208
Lockfile *lock.File

0 commit comments

Comments
 (0)