Skip to content

Commit 9d6bd19

Browse files
committed
localenv: follow include-group when scanning for the user's databricks-connect pin
Review follow-up. PEP 735 {include-group = "..."} is an indirection, so the user's pin may live in a group the dev group merely references: [dependency-groups] dev = [{include-group = "spark"}] spark = ["databricks-connect==16.1.0"] The previous scan only walked dev's own string entries, so it found no pin and emitted no W_DBCONNECT_PIN_OVERRIDDEN — while MergeManaged still injects the env's pin into dev, leaving the file with two pins for the same package and no advisory that one replaced the other. groupRequirements now resolves the reference chain, normalizing group names per PEP 503 (which PEP 735 also specifies) and skipping already-visited groups so an include-group cycle terminates. Decoding dependency-groups as a map is what makes the other groups reachable; the walk is deterministic despite the map because warning order is fixed downstream. Also scopes the environment-marker check to the post-extras remainder rather than the raw requirement, so it tests only where a marker can appear. Behavior is unchanged for every input either form decides — a ";" in a url requirement is still skipped, which costs nothing since a url has no comparable version range. This closes a false negative that predates the warnings work rather than a regression from it: the parent commit reported nothing for the same input too. Co-authored-by: Isaac
1 parent 7f28303 commit 9d6bd19

2 files changed

Lines changed: 115 additions & 21 deletions

File tree

‎libs/localenv/warnings.go‎

Lines changed: 55 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -22,16 +22,11 @@ var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`)
2222
var singleClauseRe = regexp.MustCompile(`^(>=|<=|==|~=|!=|<|>)?\s*([0-9]+(?:\.[0-9]+)*)`)
2323

2424
// splitDepSpec returns the normalized package name and the version specifier
25-
// portion of a dependency string. ok is false when there is no recognizable name,
26-
// or when the requirement carries an environment marker: a marker makes the
27-
// dependency conditional on the resolving interpreter, which we do not evaluate,
28-
// so comparing its range could flag a pin that never applies.
25+
// portion of a dependency string, with any extras stripped. ok is false when there
26+
// is no recognizable name, or when what follows the name carries an environment
27+
// marker (see below).
2928
func splitDepSpec(dep string) (name, spec string, ok bool) {
30-
dep = strings.TrimSpace(dep)
31-
if strings.Contains(dep, ";") {
32-
return "", "", false
33-
}
34-
m := depSpecRe.FindStringSubmatch(dep)
29+
m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep))
3530
if m == nil {
3631
return "", "", false
3732
}
@@ -43,6 +38,15 @@ func splitDepSpec(dep string) (name, spec string, ok bool) {
4338
spec = strings.TrimSpace(spec[i+1:])
4439
}
4540
}
41+
// A marker gates the whole requirement on the resolving interpreter, which we do
42+
// not evaluate, so a pin that may not apply must not be compared. Checking the
43+
// remainder rather than the raw string scopes this to where a marker can actually
44+
// appear, leaving an extras list that happens to contain ";" alone. A ";" inside
45+
// a url requirement still lands here, which costs nothing: a url has no
46+
// comparable version range and would be undecidable regardless.
47+
if strings.Contains(spec, ";") {
48+
return "", "", false
49+
}
4650
return normalizePackageName(m[1]), spec, true
4751
}
4852

@@ -60,20 +64,51 @@ type userPyprojectTOML struct {
6064
RequiresPython string `toml:"requires-python"`
6165
Dependencies []string `toml:"dependencies"`
6266
} `toml:"project"`
63-
DependencyGroups struct {
64-
Dev []any `toml:"dev"`
65-
} `toml:"dependency-groups"`
67+
// Every group is decoded, not just dev, because a dev entry may pull in another
68+
// group by reference — see groupRequirements.
69+
DependencyGroups map[string][]any `toml:"dependency-groups"`
6670
}
6771

68-
// devRequirements returns the requirement strings of a dependency group, skipping
69-
// PEP 735 table entries (include-group) that are not requirements themselves.
70-
func devRequirements(entries []any) []string {
71-
out := make([]string, 0, len(entries))
72-
for _, e := range entries {
73-
if s, ok := e.(string); ok {
74-
out = append(out, s)
72+
// devGroup is the dependency group whose databricks-connect pin the merge manages.
73+
const devGroup = "dev"
74+
75+
// groupRequirements returns the requirement strings reachable from the named
76+
// dependency group, following PEP 735 {include-group = "..."} indirections.
77+
//
78+
// Resolving the indirection matters because MergeManaged writes the env's pin into
79+
// dev regardless of where the user's own pin lives: with dev = [{include-group =
80+
// "spark"}] and the pin inside the spark group, a non-recursive scan reports no
81+
// override while the merged file ends up carrying two pins for the same package.
82+
// Group names are compared under PEP 503 normalization, which PEP 735 also
83+
// specifies for group names. A group already visited is skipped, so an
84+
// include-group cycle terminates instead of recursing forever.
85+
func groupRequirements(groups map[string][]any, name string) []string {
86+
byName := make(map[string][]any, len(groups))
87+
for g, entries := range groups {
88+
byName[normalizePackageName(g)] = entries
89+
}
90+
91+
var out []string
92+
visited := make(map[string]bool, len(groups))
93+
var walk func(string)
94+
walk = func(g string) {
95+
g = normalizePackageName(g)
96+
if visited[g] {
97+
return
98+
}
99+
visited[g] = true
100+
for _, e := range byName[g] {
101+
switch v := e.(type) {
102+
case string:
103+
out = append(out, v)
104+
case map[string]any:
105+
if inc, ok := v["include-group"].(string); ok {
106+
walk(inc)
107+
}
108+
}
75109
}
76110
}
111+
walk(name)
77112
return out
78113
}
79114

@@ -114,7 +149,7 @@ func detectMergeWarnings(userPyproject []byte, c Constraints) []Warning {
114149
// the merge replaces it. Only meaningful in default mode (c.DatabricksConnect
115150
// is empty in constraints-only, where the dev group is left untouched).
116151
if c.DatabricksConnect != "" {
117-
for _, entry := range devRequirements(p.DependencyGroups.Dev) {
152+
for _, entry := range groupRequirements(p.DependencyGroups, devGroup) {
118153
if !isDatabricksConnectDep(entry) {
119154
continue
120155
}

‎libs/localenv/warnings_test.go‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,62 @@ test = []
114114
}, codes(detectMergeWarnings(user, c)))
115115
}
116116

117+
func TestDetectMergeWarningsIncludeGroupIndirection(t *testing.T) {
118+
// The user's pin is reachable only through an include-group, but MergeManaged
119+
// writes the env's pin into dev anyway — so without following the indirection the
120+
// merged file would carry two databricks-connect pins and no override warning.
121+
c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"}
122+
123+
indirect := []byte(`[project]
124+
requires-python = "==3.12.*"
125+
126+
[dependency-groups]
127+
dev = [{include-group = "spark"}]
128+
spark = ["databricks-connect==16.1.0"]
129+
`)
130+
assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(indirect, c)))
131+
132+
// An include-group cycle must terminate rather than recurse forever, and the pin
133+
// behind it is still found.
134+
cyclic := []byte(`[project]
135+
requires-python = "==3.12.*"
136+
137+
[dependency-groups]
138+
dev = [{include-group = "a"}]
139+
a = [{include-group = "dev"}, "databricks-connect==16.1.0"]
140+
`)
141+
assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(cyclic, c)))
142+
143+
// PEP 735 normalizes group names the same way PEP 503 normalizes package names.
144+
renamed := []byte(`[project]
145+
requires-python = "==3.12.*"
146+
147+
[dependency-groups]
148+
dev = [{include-group = "My_Spark.Group"}]
149+
"my-spark-group" = ["databricks-connect==16.1.0"]
150+
`)
151+
assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(renamed, c)))
152+
153+
// An included pin that already matches the env is not an override.
154+
matching := []byte(`[project]
155+
requires-python = "==3.12.*"
156+
157+
[dependency-groups]
158+
dev = [{include-group = "spark"}]
159+
spark = ["databricks-connect==17.0.0"]
160+
`)
161+
assert.Empty(t, detectMergeWarnings(matching, c))
162+
163+
// A dangling reference is not an error; there is simply no pin to compare.
164+
dangling := []byte(`[project]
165+
requires-python = "==3.12.*"
166+
167+
[dependency-groups]
168+
dev = [{include-group = "absent"}]
169+
`)
170+
assert.Empty(t, detectMergeWarnings(dangling, c))
171+
}
172+
117173
func TestConstraintConflictsDuplicateEnvEntriesAreOrderIndependent(t *testing.T) {
118174
// Constraint entries for one package compose as a conjunction. Joining them
119175
// yields a multi-clause spec, which is an unknown range — so neither ordering
@@ -132,9 +188,12 @@ func TestSplitDepSpecExtrasAndMarkers(t *testing.T) {
132188
assert.Equal(t, "==17.0.0", spec)
133189

134190
// A marker makes the requirement conditional on the resolving interpreter,
135-
// which we do not evaluate — so it is skipped rather than compared.
191+
// which we do not evaluate — so it is skipped rather than compared, with or
192+
// without extras in front of it.
136193
_, _, ok = splitDepSpec(`pyarrow==17.0.0; python_version < "3.12"`)
137194
assert.False(t, ok)
195+
_, _, ok = splitDepSpec(`pyarrow[compute]==17.0.0; python_version < "3.12"`)
196+
assert.False(t, ok)
138197

139198
// PEP 503 normalization applies to the name on both sides of the lookup.
140199
name, spec, ok = splitDepSpec("PyArrow_Extra ==1.0")

0 commit comments

Comments
 (0)