Skip to content

Commit 7ab2d35

Browse files
ipinceipince
authored
[bugs]: add other sysinfos, but respect existing ca paths (#1315)
## Summary This should fix the two remaining bugs in the lockfile merging logic: 1. If we have new system infos for a system that is not our own, add them anyway. 2. If we have a system info whose StorePath does not match the existing info's StorePath, then replace the info. This is to ensure correctness--all infos should come from the same resolved package version. ## How was it tested? Added unit tests!
1 parent 6746296 commit 7ab2d35

File tree

3 files changed

+222
-10
lines changed

3 files changed

+222
-10
lines changed

‎internal/boxcli/featureflag/feature.go‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ package featureflag
66
import (
77
"os"
88
"strconv"
9+
"testing"
910

1011
"go.jetpack.io/devbox/internal/debug"
1112
"go.jetpack.io/devbox/internal/envir"
@@ -54,6 +55,10 @@ func (f *feature) Enabled() bool {
5455
return f.enabled
5556
}
5657

58+
func (f *feature) EnableForTest(t *testing.T) {
59+
t.Setenv(envir.DevboxFeaturePrefix+f.name, "1")
60+
}
61+
5762
// All returns a map of all known features flags and whether they're enabled.
5863
func All() map[string]bool {
5964
m := make(map[string]bool, len(features))

‎internal/impl/update.go‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -128,19 +128,34 @@ func (d *Devbox) mergeResolvedPackageToLockfile(
128128
return err
129129
}
130130

131-
// If the resolved has a system info for the user's system, then add/overwrite it. We don't overwrite
132-
// other system infos because we don't want to clobber system-dependent CAStorePaths.
133-
if newSysInfo, ok := resolved.Systems[userSystem]; ok {
134-
if !newSysInfo.Equals(existing.Systems[userSystem]) {
135-
// We only guard this so that the ux messaging is accurate. We could overwrite every time.
136-
if lockfile.Packages[pkg.Raw].Systems == nil {
137-
lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
131+
if lockfile.Packages[pkg.Raw].Systems == nil {
132+
lockfile.Packages[pkg.Raw].Systems = map[string]*lock.SystemInfo{}
133+
}
134+
135+
updated := false
136+
for sysName, newSysInfo := range resolved.Systems {
137+
if sysName == userSystem {
138+
// The resolved pkg has a system info for the user's system, so add/overwrite it.
139+
if !newSysInfo.Equals(existing.Systems[userSystem]) {
140+
// We only guard this so that the ux messaging is accurate. We could overwrite every time.
141+
lockfile.Packages[pkg.Raw].Systems[userSystem] = newSysInfo
142+
updated = true
143+
}
144+
} else {
145+
// Add other system infos if they don't exist, or if we have a different StorePath. This may
146+
// overwrite an existing CAPath, but to ensure correctness we should ensure that all StorePaths
147+
// come from the same package version.
148+
existingSysInfo, exists := existing.Systems[sysName]
149+
if !exists || existingSysInfo.StorePath != newSysInfo.StorePath {
150+
lockfile.Packages[pkg.Raw].Systems[sysName] = newSysInfo
151+
updated = true
138152
}
139-
lockfile.Packages[pkg.Raw].Systems[userSystem] = newSysInfo
140-
ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
141-
return nil
142153
}
143154
}
155+
if updated {
156+
ux.Finfo(d.writer, "Updated system information for %s\n", pkg)
157+
return nil
158+
}
144159
}
145160

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

‎internal/impl/update_test.go‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,10 @@ import (
55
"testing"
66

77
"github.com/stretchr/testify/require"
8+
"go.jetpack.io/devbox/internal/boxcli/featureflag"
89
"go.jetpack.io/devbox/internal/devpkg"
910
"go.jetpack.io/devbox/internal/lock"
11+
"go.jetpack.io/devbox/internal/nix"
1012
)
1113

1214
func TestUpdateNewPackageIsAdded(t *testing.T) {
@@ -28,3 +30,193 @@ func TestUpdateNewPackageIsAdded(t *testing.T) {
2830

2931
require.Contains(t, lockfile.Packages, raw)
3032
}
33+
34+
func TestUpdateNewCurrentSysInfoIsAdded(t *testing.T) {
35+
featureflag.RemoveNixpkgs.EnableForTest(t)
36+
devbox := devboxForTesting(t)
37+
38+
raw := "[email protected]"
39+
sys := currentSystem(t)
40+
devPkg := &devpkg.Package{
41+
Raw: raw,
42+
}
43+
resolved := &lock.Package{
44+
Resolved: "resolved-flake-reference",
45+
Systems: map[string]*lock.SystemInfo{
46+
sys: {
47+
StorePath: "store_path1",
48+
CAStorePath: "ca_path1",
49+
},
50+
},
51+
}
52+
lockfile := &lock.File{
53+
Packages: map[string]*lock.Package{
54+
raw: {
55+
Resolved: "resolved-flake-reference",
56+
// No system infos.
57+
},
58+
},
59+
}
60+
61+
err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile)
62+
require.NoError(t, err, "update failed")
63+
64+
require.Contains(t, lockfile.Packages, raw)
65+
require.Contains(t, lockfile.Packages[raw].Systems, sys)
66+
require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys].StorePath)
67+
require.Equal(t, "ca_path1", lockfile.Packages[raw].Systems[sys].CAStorePath)
68+
}
69+
70+
func TestUpdateNewSysInfoIsAdded(t *testing.T) {
71+
featureflag.RemoveNixpkgs.EnableForTest(t)
72+
devbox := devboxForTesting(t)
73+
74+
raw := "[email protected]"
75+
sys1 := currentSystem(t)
76+
sys2 := "system2"
77+
devPkg := &devpkg.Package{
78+
Raw: raw,
79+
}
80+
resolved := &lock.Package{
81+
Resolved: "resolved-flake-reference",
82+
Systems: map[string]*lock.SystemInfo{
83+
sys1: {
84+
StorePath: "store_path1",
85+
CAStorePath: "ca_path1",
86+
},
87+
sys2: {
88+
StorePath: "store_path2",
89+
},
90+
},
91+
}
92+
lockfile := &lock.File{
93+
Packages: map[string]*lock.Package{
94+
raw: {
95+
Resolved: "resolved-flake-reference",
96+
Systems: map[string]*lock.SystemInfo{
97+
sys1: {
98+
StorePath: "store_path1",
99+
CAStorePath: "ca_path1",
100+
},
101+
// Missing sys2
102+
},
103+
},
104+
},
105+
}
106+
107+
err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile)
108+
require.NoError(t, err, "update failed")
109+
110+
require.Contains(t, lockfile.Packages, raw)
111+
require.Contains(t, lockfile.Packages[raw].Systems, sys1)
112+
require.Contains(t, lockfile.Packages[raw].Systems, sys2)
113+
require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].StorePath)
114+
}
115+
116+
func TestUpdateOtherSysInfoIsReplaced(t *testing.T) {
117+
featureflag.RemoveNixpkgs.EnableForTest(t)
118+
devbox := devboxForTesting(t)
119+
120+
raw := "[email protected]"
121+
sys1 := currentSystem(t)
122+
sys2 := "system2"
123+
devPkg := &devpkg.Package{
124+
Raw: raw,
125+
}
126+
resolved := &lock.Package{
127+
Resolved: "resolved-flake-reference",
128+
Systems: map[string]*lock.SystemInfo{
129+
sys1: {
130+
StorePath: "store_path1",
131+
CAStorePath: "ca_path1",
132+
},
133+
sys2: {
134+
StorePath: "store_path2",
135+
},
136+
},
137+
}
138+
lockfile := &lock.File{
139+
Packages: map[string]*lock.Package{
140+
raw: {
141+
Resolved: "resolved-flake-reference",
142+
Systems: map[string]*lock.SystemInfo{
143+
sys1: {
144+
StorePath: "store_path1",
145+
CAStorePath: "ca_path1",
146+
},
147+
sys2: {
148+
StorePath: "mismatching_store_path",
149+
CAStorePath: "ca_path2",
150+
},
151+
},
152+
},
153+
},
154+
}
155+
156+
err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile)
157+
require.NoError(t, err, "update failed")
158+
159+
require.Contains(t, lockfile.Packages, raw)
160+
require.Contains(t, lockfile.Packages[raw].Systems, sys1)
161+
require.Contains(t, lockfile.Packages[raw].Systems, sys2)
162+
require.Equal(t, "store_path1", lockfile.Packages[raw].Systems[sys1].StorePath)
163+
require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].StorePath)
164+
require.Empty(t, lockfile.Packages[raw].Systems[sys2].CAStorePath)
165+
}
166+
167+
func TestUpdateCAPathIsNotReplaced(t *testing.T) {
168+
featureflag.RemoveNixpkgs.EnableForTest(t)
169+
devbox := devboxForTesting(t)
170+
171+
raw := "[email protected]"
172+
sys1 := currentSystem(t)
173+
sys2 := "system2"
174+
devPkg := &devpkg.Package{
175+
Raw: raw,
176+
}
177+
resolved := &lock.Package{
178+
Resolved: "resolved-flake-reference",
179+
Systems: map[string]*lock.SystemInfo{
180+
sys1: {
181+
StorePath: "store_path1",
182+
CAStorePath: "ca_path1",
183+
},
184+
sys2: {
185+
StorePath: "store_path2",
186+
// No CAPath here because this is not the current system.
187+
},
188+
},
189+
}
190+
lockfile := &lock.File{
191+
Packages: map[string]*lock.Package{
192+
raw: {
193+
Resolved: "resolved-flake-reference",
194+
Systems: map[string]*lock.SystemInfo{
195+
sys1: {
196+
StorePath: "store_path1",
197+
CAStorePath: "ca_path1",
198+
},
199+
sys2: {
200+
StorePath: "store_path2",
201+
CAStorePath: "ca_path2", // we already have CAPath for this system; it should not be replaced
202+
},
203+
},
204+
},
205+
},
206+
}
207+
208+
err := devbox.mergeResolvedPackageToLockfile(context.Background(), devPkg, resolved, lockfile)
209+
require.NoError(t, err, "update failed")
210+
211+
require.Contains(t, lockfile.Packages, raw)
212+
require.Contains(t, lockfile.Packages[raw].Systems, sys1)
213+
require.Contains(t, lockfile.Packages[raw].Systems, sys2)
214+
require.Equal(t, "store_path2", lockfile.Packages[raw].Systems[sys2].StorePath)
215+
require.Equal(t, "ca_path2", lockfile.Packages[raw].Systems[sys2].CAStorePath)
216+
}
217+
218+
func currentSystem(t *testing.T) string {
219+
sys, err := nix.System() // NOTE: we could mock this too, if it helps.
220+
require.NoError(t, err)
221+
return sys
222+
}

0 commit comments

Comments
 (0)