From bce69afa4a083ef031356f3dee265766bf81b43c Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Wed, 5 Aug 2026 16:15:31 +0200 Subject: [PATCH 1/3] localenv: populate setup-local warnings and durationMs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both fields were declared in the --output json contract but never populated: warnings was always [] and durationMs always 0. They back the ERD's "setup result" metrics (merge-quality warning count + the ~3min duration claim), so the extension had nothing real to report. DECO-27875. - durationMs: add an injectable clock (Pipeline.Now, default time.Now) and stamp wall time across every Run exit path. Tests inject a fixed clock; the acceptance JSON goldens normalize the value via a [DURATION_MS] repl so they stay deterministic. - warnings: emit a documented, closed set of categorical codes from the merge phase — W_REQUIRES_PYTHON_OVERRIDDEN and W_DBCONNECT_PIN_OVERRIDDEN when the user's pins differ from the env's and are replaced, and W_USER_CONSTRAINT_CONFLICT when a [project].dependencies pin is provably disjoint from the env's constraint for that package. Detection is a read-only compare of the pre-merge pyproject against the fetched constraints (detectMergeWarnings); MergeManaged still owns the byte edits. The conflict check is conservative PEP 440 (== / ~= / bounds) and only fires on a provable disjoint, so an ambiguous range is never a false positive — uv remains the real resolver. Warnings fire for both --dry-run and real runs, and only for existing projects (greenfield has nothing of the user's to override). Contract-additive and scoped to libs/localenv; the command is still hidden. DECO-27875 Co-authored-by: Isaac --- .../cluster-name-ambiguous-json/output.txt | 2 +- .../localenv/constraints-only/output.txt | 2 +- .../localenv/flag-conflict-json/output.txt | 2 +- acceptance/localenv/json-error/output.txt | 2 +- .../merge-warnings-json/out.test.toml | 3 + .../localenv/merge-warnings-json/output.txt | 68 +++++ .../localenv/merge-warnings-json/script | 14 + .../localenv/merge-warnings-json/test.toml | 21 ++ .../localenv/serverless-json/output.txt | 2 +- acceptance/test.toml | 7 + libs/localenv/constraints.go | 3 +- libs/localenv/pipeline.go | 25 ++ libs/localenv/pipeline_test.go | 65 +++++ libs/localenv/result.go | 30 +- libs/localenv/warnings.go | 274 ++++++++++++++++++ libs/localenv/warnings_test.go | 114 ++++++++ 16 files changed, 623 insertions(+), 11 deletions(-) create mode 100644 acceptance/localenv/merge-warnings-json/out.test.toml create mode 100644 acceptance/localenv/merge-warnings-json/output.txt create mode 100644 acceptance/localenv/merge-warnings-json/script create mode 100644 acceptance/localenv/merge-warnings-json/test.toml create mode 100644 libs/localenv/warnings.go create mode 100644 libs/localenv/warnings_test.go diff --git a/acceptance/localenv/cluster-name-ambiguous-json/output.txt b/acceptance/localenv/cluster-name-ambiguous-json/output.txt index ff557c87dea..cb5dcf04df7 100644 --- a/acceptance/localenv/cluster-name-ambiguous-json/output.txt +++ b/acceptance/localenv/cluster-name-ambiguous-json/output.txt @@ -38,5 +38,5 @@ "message": "resolving cluster name \"dup\": there are 2 active clusters named \"dup\"; use --cluster-id to disambiguate", "diskMutated": false }, - "durationMs": 0 + "durationMs": [DURATION_MS] } diff --git a/acceptance/localenv/constraints-only/output.txt b/acceptance/localenv/constraints-only/output.txt index e7a1c76529c..a98729c6ac2 100644 --- a/acceptance/localenv/constraints-only/output.txt +++ b/acceptance/localenv/constraints-only/output.txt @@ -49,5 +49,5 @@ ], "warnings": [], "error": null, - "durationMs": 0 + "durationMs": [DURATION_MS] } diff --git a/acceptance/localenv/flag-conflict-json/output.txt b/acceptance/localenv/flag-conflict-json/output.txt index 84840872e3f..5090f47dd70 100644 --- a/acceptance/localenv/flag-conflict-json/output.txt +++ b/acceptance/localenv/flag-conflict-json/output.txt @@ -38,5 +38,5 @@ "message": "invalid compute target flags: flags --cluster-id and --serverless-version are mutually exclusive; specify at most one", "diskMutated": false }, - "durationMs": 0 + "durationMs": [DURATION_MS] } diff --git a/acceptance/localenv/json-error/output.txt b/acceptance/localenv/json-error/output.txt index b2e5a0ba43e..7ea96c75f2d 100644 --- a/acceptance/localenv/json-error/output.txt +++ b/acceptance/localenv/json-error/output.txt @@ -38,5 +38,5 @@ "message": "No compute target is selected. Select a cluster or serverless target, or pass --cluster-id / --cluster-name / --serverless-version / --job-task", "diskMutated": false }, - "durationMs": 0 + "durationMs": [DURATION_MS] } diff --git a/acceptance/localenv/merge-warnings-json/out.test.toml b/acceptance/localenv/merge-warnings-json/out.test.toml new file mode 100644 index 00000000000..e90b6d5d1ba --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/out.test.toml @@ -0,0 +1,3 @@ +Local = true +Cloud = false +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] diff --git a/acceptance/localenv/merge-warnings-json/output.txt b/acceptance/localenv/merge-warnings-json/output.txt new file mode 100644 index 00000000000..d0823b671c1 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/output.txt @@ -0,0 +1,68 @@ + +>>> [CLI] environments setup-local --serverless-version 4 --dry-run --output json +{ + "schemaVersion": 1, + "command": "environments setup-local", + "ok": true, + "mode": "default", + "dryRun": true, + "compute": { + "source": "serverless", + "serverlessVersion": "v4", + "envKey": "serverless/serverless-v4" + }, + "resolved": { + "pythonVersion": "3.12", + "dbconnectVersion": "17.2.0", + "artifactSource": "network" + }, + "greenfield": false, + "plan": { + "wouldWrite": "[TEST_TMP_DIR]/pyproject.toml", + "wouldBackup": "[TEST_TMP_DIR]/pyproject.toml.bak", + "wouldInstallPython": "3.12", + "diff": "--- pyproject.toml\n+++ pyproject.toml.new\n@@ -1,7 +1,15 @@\n [project]\n name = \"existing\"\n-requires-python = \"\u003e=3.10\"\n+requires-python = \"==3.12.*\"\n dependencies = [\"pyarrow==17.0.0\"]\n \n [dependency-groups]\n-dev = [\"databricks-connect~=16.1.0\"]\n+dev = [\"databricks-connect~=17.2.0\"]\n+\n+# managed by databricks environments setup-local — do not edit\n+[tool.uv]\n+constraint-dependencies = [\n+ \"pyarrow~=21.0.0\",\n+ \"pandas\u003c3\",\n+]\n+# end managed by databricks environments setup-local\n" + }, + "phases": [ + { + "phase": "preflight", + "status": "ok" + }, + { + "phase": "resolve", + "status": "ok" + }, + { + "phase": "fetch", + "status": "ok" + }, + { + "phase": "merge", + "status": "ok" + }, + { + "phase": "provision", + "status": "ok" + }, + { + "phase": "validate", + "status": "ok" + } + ], + "warnings": [ + { + "code": "W_REQUIRES_PYTHON_OVERRIDDEN", + "message": "requires-python \"\u003e=3.10\" was replaced by the environment's \"==3.12.*\"" + }, + { + "code": "W_DBCONNECT_PIN_OVERRIDDEN", + "message": "databricks-connect \"databricks-connect~=16.1.0\" was replaced by the environment's \"databricks-connect~=17.2.0\"" + }, + { + "code": "W_USER_CONSTRAINT_CONFLICT", + "message": "dependency \"pyarrow==17.0.0\" conflicts with the environment constraint \"pyarrow~=21.0.0\"" + } + ], + "error": null, + "durationMs": [DURATION_MS] +} diff --git a/acceptance/localenv/merge-warnings-json/script b/acceptance/localenv/merge-warnings-json/script new file mode 100644 index 00000000000..774617b38b3 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/script @@ -0,0 +1,14 @@ +# A pre-existing project whose pins conflict with the env constraints: an older +# requires-python and databricks-connect, plus a dependency pinned outside the +# env's constraint range. The dry-run merge should surface all three warnings. +cat > pyproject.toml <<'PYPROJECT' +[project] +name = "existing" +requires-python = ">=3.10" +dependencies = ["pyarrow==17.0.0"] + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +PYPROJECT + +trace $CLI environments setup-local --serverless-version 4 --dry-run --output json diff --git a/acceptance/localenv/merge-warnings-json/test.toml b/acceptance/localenv/merge-warnings-json/test.toml new file mode 100644 index 00000000000..c75ca1071f4 --- /dev/null +++ b/acceptance/localenv/merge-warnings-json/test.toml @@ -0,0 +1,21 @@ +EnvMatrix.DATABRICKS_BUNDLE_ENGINE = ["direct"] + +# The script writes a pre-existing pyproject.toml to trigger the merge path; it is +# an input the test creates, not a golden to compare. +Ignore = ["pyproject.toml"] + +[Env] +DATABRICKS_LOCALENV_CONSTRAINT_SOURCE_URL_TEST_OVERRIDE = "$DATABRICKS_HOST" + +[[Server]] +Pattern = "GET /serverless/serverless-v4/pyproject.toml" +Response.Body = ''' +[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=17.2.0"] + +[tool.uv] +constraint-dependencies = ["pyarrow~=21.0.0", "pandas<3"] +''' diff --git a/acceptance/localenv/serverless-json/output.txt b/acceptance/localenv/serverless-json/output.txt index b0bc441a7c5..4b46fda397d 100644 --- a/acceptance/localenv/serverless-json/output.txt +++ b/acceptance/localenv/serverless-json/output.txt @@ -50,5 +50,5 @@ ], "warnings": [], "error": null, - "durationMs": 0 + "durationMs": [DURATION_MS] } diff --git a/acceptance/test.toml b/acceptance/test.toml index 124c79f0ba5..c01c8cb7af7 100644 --- a/acceptance/test.toml +++ b/acceptance/test.toml @@ -90,3 +90,10 @@ Order = 9 [[Repls]] Old = "deco-uc-prod-isolated-aws-us-east-1|metastore_azure_eastus2" New = "[METASTORE_NAME]" + +[[Repls]] +# environments setup-local emits a real wall-time durationMs; normalize it so the +# JSON goldens are deterministic. Runs before the generic numeric repls (Order 10). +Old = '"durationMs": \d+' +New = '"durationMs": [DURATION_MS]' +Order = 8 diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index eae55263a6e..08a6c311427 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -236,7 +236,8 @@ func fetchURL(ctx context.Context, url string) ([]byte, error) { // pyprojectTOML mirrors the pyproject.toml fields we care about. type pyprojectTOML struct { Project struct { - RequiresPython string `toml:"requires-python"` + RequiresPython string `toml:"requires-python"` + Dependencies []string `toml:"dependencies"` } `toml:"project"` DependencyGroups struct { Dev []string `toml:"dev"` diff --git a/libs/localenv/pipeline.go b/libs/localenv/pipeline.go index 30f803a21e4..f5d39807940 100644 --- a/libs/localenv/pipeline.go +++ b/libs/localenv/pipeline.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "time" "github.com/databricks/cli/libs/log" "github.com/hexops/gotextdiff" @@ -53,10 +54,23 @@ type Pipeline struct { Compute ComputeClient PM PackageManager + // Now returns the current time; it exists so tests can inject a deterministic + // clock (acceptance goldens would otherwise carry a real, changing durationMs). + // nil means time.Now — see now(). + Now func() time.Time + // res accumulates phase statuses and result fields as the run progresses. res *Result } +// now returns the current time via the injected clock, defaulting to time.Now. +func (p *Pipeline) now() time.Time { + if p.Now != nil { + return p.Now() + } + return time.Now() +} + // Run executes all pipeline phases in order and returns a fully populated Result. // On a phase error, Result.Error is set and the same error is also returned. The // Result always carries the full canonical phase list: phases completed before a @@ -81,6 +95,12 @@ func (p *Pipeline) Run(ctx context.Context) (*Result, error) { // Phases start as pending and flip to ok/error as the run progresses. p.res.Phases = initialPhases() + // Measure wall time across every exit path (success and failure alike) so the + // --json durationMs reflects the whole pipeline. Injected clock keeps it + // deterministic under test. + start := p.now() + defer func() { p.res.DurationMs = p.now().Sub(start).Milliseconds() }() + if err := p.run(ctx); err != nil { // A cancelled context means the user or parent interrupted us (SIGINT/ // SIGTERM). The phase that was running reports its own failure (e.g. uv @@ -307,6 +327,11 @@ func (p *Pipeline) mergePlan(_ context.Context, pyMinor string, c *Constraints, if err != nil { return nil, greenfield, p.fail(PhaseMerge, false, NewError(ErrMerge, err, "merge managed regions failed")) } + // Surface merge-quality warnings (overridden pins, conflicting user + // constraints) from the pre-merge file. Greenfield has nothing of the + // user's to override, so it is skipped. This runs for both dry-run and real + // runs so the --json consumer sees the same warnings either way. + p.res.Warnings = append(p.res.Warnings, detectMergeWarnings(baseBytes, effective)...) } // Under --dry-run, build the plan (with a diff) for reporting. A real run does diff --git a/libs/localenv/pipeline_test.go b/libs/localenv/pipeline_test.go index 865e4e30263..ff6ce3ceeeb 100644 --- a/libs/localenv/pipeline_test.go +++ b/libs/localenv/pipeline_test.go @@ -10,6 +10,7 @@ import ( "runtime" "strings" "testing" + "time" "github.com/databricks/cli/libs/process" "github.com/stretchr/testify/assert" @@ -221,6 +222,70 @@ func TestPipelineReportsCancellationNotProvisionFailure(t *testing.T) { assert.NotContains(t, pe.Error(), "\n", "the error must stay single-line") } +func TestPipelineSurfacesMergeWarnings(t *testing.T) { + // writeProject pins requires-python ">=3.10" and databricks-connect ~=16.0.0, + // while the server's constraint pins "==3.12.*" and ~=17.2.0 — so a merge + // overrides both, and the result must carry the two override warnings. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.Equal(t, []string{WarnRequiresPythonOverridden, WarnDBConnectPinOverridden}, codes(res.Warnings)) +} + +func TestPipelineGreenfieldHasNoWarnings(t *testing.T) { + // A greenfield project has nothing of the user's to override. + dir := t.TempDir() + srv := newTestServer(t) + defer srv.Close() + + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.Empty(t, res.Warnings) + assert.True(t, res.Greenfield) +} + +func TestPipelineReportsDurationFromInjectedClock(t *testing.T) { + // The injected clock advances 250ms between the first call (start) and the + // second (deferred end), so durationMs is deterministic — not 0, not wall time. + dir := writeProject(t) + srv := newTestServer(t) + defer srv.Close() + + base := time.Unix(1_700_000_000, 0) + var calls int + p := &Pipeline{ + Mode: ModeDefault, Check: true, ProjectDir: dir, + ConstraintBaseURL: srv.URL, CacheDir: t.TempDir(), + Flags: ComputeFlags{Serverless: "v4"}, + Compute: stubCompute{}, PM: fakePM{py: "3.12", dbc: "17.2.0"}, + Now: func() time.Time { + calls++ + if calls == 1 { + return base + } + return base.Add(250 * time.Millisecond) + }, + } + res, err := p.Run(t.Context()) + require.NoError(t, err) + assert.Equal(t, int64(250), res.DurationMs) +} + func TestPipelineCheckReRunPlanMatchesRealRun(t *testing.T) { // On a re-run where the .bak already exists and the live file already equals // the merged output, --dry-run must report a plan a real run would perform: no diff --git a/libs/localenv/result.go b/libs/localenv/result.go index 244769727af..9208efe3e5f 100644 --- a/libs/localenv/result.go +++ b/libs/localenv/result.go @@ -189,12 +189,32 @@ type PhaseStatus struct { Detail string `json:"-"` } -// Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). +// Warning is a non-fatal advisory surfaced in --json "warnings" (spec §6). Code +// is a stable, categorical identifier from the closed set below; Message is +// human-readable text for the text renderer and is not part of the contract. type Warning struct { Code string `json:"code"` Message string `json:"message"` } +// Warning codes are the closed, categorical set surfaced in --json warnings[]. +// They let a consumer report a count and a code histogram (merge quality) without +// parsing free-form text. All are emitted from the merge phase, where the fetched +// env-owned pins can conflict with what the user already had. +const ( + // WarnRequiresPythonOverridden: the user's [project].requires-python differed + // from the env's pin and was replaced by the managed value. + WarnRequiresPythonOverridden = "W_REQUIRES_PYTHON_OVERRIDDEN" + // WarnDBConnectPinOverridden: the user's databricks-connect pin differed from + // the env's pin and was replaced by the managed value. + WarnDBConnectPinOverridden = "W_DBCONNECT_PIN_OVERRIDDEN" + // WarnUserConstraintConflict: a [project].dependencies entry pins a package + // that the env's constraint-dependencies also constrains, to a provably + // non-overlapping version range (uv will likely fail to resolve). Emitted only + // when the ranges are provably disjoint; ambiguous cases are not flagged. + WarnUserConstraintConflict = "W_USER_CONSTRAINT_CONFLICT" +) + // Result is the full outcome of a sync run and the root of the --json object // (spec §6). Field order matches the spec's schema so JSON key order is stable. // @@ -217,10 +237,10 @@ type Result struct { Warnings []Warning `json:"warnings"` Error *PipelineError `json:"error"` BackupPath string `json:"backupPath,omitempty"` - // DurationMs is part of the §6 contract but reserved for now: the pipeline - // does not measure wall time (a real clock would make acceptance goldens - // non-deterministic), so it is always emitted as 0 until timing is wired - // through a clock the tests can control. + // DurationMs is the pipeline's wall time in milliseconds (spec §6), measured + // through Pipeline.now so tests can inject a deterministic clock. It covers the + // CLI pipeline only; the extension measures its own end-to-end latency (process + // spawn, interpreter adoption) separately. DurationMs int64 `json:"durationMs"` } diff --git a/libs/localenv/warnings.go b/libs/localenv/warnings.go new file mode 100644 index 00000000000..c7565a0ba25 --- /dev/null +++ b/libs/localenv/warnings.go @@ -0,0 +1,274 @@ +package localenv + +import ( + "fmt" + "regexp" + "slices" + "strconv" + "strings" + + "github.com/BurntSushi/toml" +) + +// depSpecRe splits a dependency string into its package name and the trailing +// version specifier (e.g. "pyarrow~=19.0" -> "pyarrow", "~=19.0"). It stops the +// name at the first PEP 508 separator; extras/markers/urls after the version are +// out of scope for the conservative conflict check. +var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`) + +// singleClauseRe parses one version clause: an operator and a dotted numeric +// release. Pre/post/dev suffixes and wildcards are not modeled — a clause we +// cannot parse this simply is treated as "unknown" and never yields a conflict. +var singleClauseRe = regexp.MustCompile(`^(>=|<=|==|~=|!=|<|>)?\s*([0-9]+(?:\.[0-9]+)*)`) + +// splitDepSpec returns the normalized package name and the raw version specifier +// portion of a dependency string. ok is false when there is no recognizable name. +func splitDepSpec(dep string) (name, spec string, ok bool) { + m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep)) + if m == nil { + return "", "", false + } + return normalizePackageName(m[1]), strings.TrimSpace(m[2]), true +} + +// detectMergeWarnings compares a user's existing pyproject.toml against the +// fetched env constraints and returns the categorical warnings the merge phase +// surfaces (spec §6 warnings[]). It is a read-only comparison run alongside the +// merge; MergeManaged still owns the actual byte edits. +// +// It is best-effort: unparseable input yields no warnings rather than an error, +// because a warning is advisory and must never fail the run. Greenfield projects +// (no pre-existing content) produce nothing — there is nothing of the user's to +// override. Warnings are deterministic and ordered (requires-python, then +// databricks-connect, then constraint conflicts sorted by package) so goldens are +// stable. +func detectMergeWarnings(userPyproject []byte, c Constraints) []Warning { + if len(userPyproject) == 0 { + return nil + } + var p pyprojectTOML + if err := toml.Unmarshal(userPyproject, &p); err != nil { + return nil + } + + var warnings []Warning + + // The user pinned a requires-python that differs from the env's pin; the merge + // replaces it with the managed value. + if up := strings.TrimSpace(p.Project.RequiresPython); up != "" && c.RequiresPython != "" && up != strings.TrimSpace(c.RequiresPython) { + warnings = append(warnings, Warning{ + Code: WarnRequiresPythonOverridden, + Message: fmt.Sprintf("requires-python %q was replaced by the environment's %q", up, c.RequiresPython), + }) + } + + // The user pinned databricks-connect to something other than the env's pin; + // the merge replaces it. Only meaningful in default mode (c.DatabricksConnect + // is empty in constraints-only, where the dev group is left untouched). + if c.DatabricksConnect != "" { + for _, entry := range p.DependencyGroups.Dev { + if !isDatabricksConnectDep(entry) { + continue + } + if strings.TrimSpace(entry) != strings.TrimSpace(c.DatabricksConnect) { + warnings = append(warnings, Warning{ + Code: WarnDBConnectPinOverridden, + Message: fmt.Sprintf("databricks-connect %q was replaced by the environment's %q", strings.TrimSpace(entry), c.DatabricksConnect), + }) + } + break + } + } + + warnings = append(warnings, constraintConflicts(p.Project.Dependencies, c.ConstraintDeps)...) + return warnings +} + +// constraintConflicts flags each user [project].dependencies pin that the env's +// constraint-dependencies also constrains to a provably non-overlapping version. +// It is deliberately conservative — it only fires when the two ranges are +// provably disjoint (see rangesDisjoint); an ambiguous pair yields nothing, so a +// false "conflict" is never reported. Results are sorted by package name for +// deterministic output. +func constraintConflicts(userDeps, envConstraints []string) []Warning { + if len(userDeps) == 0 || len(envConstraints) == 0 { + return nil + } + // Index the env constraints by normalized package name. + envByName := make(map[string]string, len(envConstraints)) + for _, ec := range envConstraints { + if name, spec, ok := splitDepSpec(ec); ok && spec != "" { + envByName[name] = spec + } + } + if len(envByName) == 0 { + return nil + } + + var warnings []Warning + for _, ud := range userDeps { + name, userSpec, ok := splitDepSpec(ud) + if !ok || userSpec == "" { + continue + } + envSpec, ok := envByName[name] + if !ok { + continue + } + if rangesDisjoint(userSpec, envSpec) { + warnings = append(warnings, Warning{ + Code: WarnUserConstraintConflict, + Message: fmt.Sprintf("dependency %q conflicts with the environment constraint %q", strings.TrimSpace(ud), name+envSpec), + }) + } + } + // Sort by package name (embedded in Message after "dependency ") for stable + // output; the count and code are what the contract carries, order is cosmetic. + sortWarningsByMessage(warnings) + return warnings +} + +// sortWarningsByMessage orders warnings by their Message so multiple conflicts +// render deterministically (map iteration and slice order upstream are unstable). +func sortWarningsByMessage(w []Warning) { + slices.SortStableFunc(w, func(a, b Warning) int { return strings.Compare(a.Message, b.Message) }) +} + +// clause is a parsed single version clause: an operator and a numeric release. +type clause struct { + op string + rel []int +} + +// parseClause parses the first "" out of a specifier. ok is false +// when the specifier has multiple comma clauses or cannot be parsed simply — both +// are treated as "unknown range", which never produces a conflict. +func parseClause(spec string) (clause, bool) { + spec = strings.TrimSpace(spec) + if spec == "" || strings.Contains(spec, ",") { + return clause{}, false + } + m := singleClauseRe.FindStringSubmatch(spec) + if m == nil { + return clause{}, false + } + // Reject anything trailing the numeric release (wildcards, pre/post/dev tags): + // modeling those correctly is out of scope, and guessing risks a false conflict. + if strings.TrimSpace(spec[len(m[0]):]) != "" { + return clause{}, false + } + op := m[1] + if op == "" { + op = "==" // a bare version in dependencies means an exact pin + } + var rel []int + for part := range strings.SplitSeq(m[2], ".") { + n, err := strconv.Atoi(part) + if err != nil { + return clause{}, false + } + rel = append(rel, n) + } + return clause{op: op, rel: rel}, true +} + +// rangesDisjoint reports whether two version specifiers provably share no +// satisfying version. It is intentionally partial: it returns true only for +// operator pairs it can decide with certainty (== / ~= / >= / > / <= / <). Any +// pair it cannot decide — a "!=", a multi-clause range, an unparseable release — +// returns false, so an uncertain case is never reported as a conflict. uv remains +// the real resolver; this only surfaces the obvious clashes as an advisory. +func rangesDisjoint(userSpec, envSpec string) bool { + a, aok := parseClause(userSpec) + b, bok := parseClause(envSpec) + if !aok || !bok { + return false + } + // Normalize so "exact-ish" (== / ~=) comparisons can be done against the other + // side regardless of argument order. + return clausesDisjoint(a, b) || clausesDisjoint(b, a) +} + +// clausesDisjoint decides disjointness treating a as the reference. It handles +// the exact-pin and bound cases; combinations it cannot decide return false. +func clausesDisjoint(a, b clause) bool { + switch a.op { + case "==": + // a pins exactly a.rel; disjoint iff that version does not satisfy b. + return !satisfies(b, a.rel) + case "~=": + // ~=X.Y admits [X.Y, X+1.0); if b is an exact pin, decide by membership. + if b.op == "==" { + return !compatibleReleaseContains(a.rel, b.rel) + } + } + return false +} + +// satisfies reports whether version v meets clause c. Only the operators used by +// clausesDisjoint's exact-pin path are decided; others conservatively return true +// (i.e. "assume satisfiable", so no conflict is reported). +func satisfies(c clause, v []int) bool { + cmp := compareRelease(v, c.rel) + switch c.op { + case "==": + return cmp == 0 + case ">=": + return cmp >= 0 + case ">": + return cmp > 0 + case "<=": + return cmp <= 0 + case "<": + return cmp < 0 + case "~=": + return compatibleReleaseContains(c.rel, v) + default: + return true + } +} + +// compatibleReleaseContains reports whether v falls in the ~=base compatible +// range: v >= base and v shares base's leading release components up to the last +// (e.g. ~=2.4.4 admits [2.4.4, 2.5.0)). base must have at least two components. +func compatibleReleaseContains(base, v []int) bool { + if len(base) < 2 { + return false + } + if compareRelease(v, base) < 0 { + return false + } + // The upper bound holds all but the last component of base fixed. + for i := range len(base) - 1 { + var vi int + if i < len(v) { + vi = v[i] + } + if vi != base[i] { + return false + } + } + return true +} + +// compareRelease compares two dotted numeric releases component-wise, treating a +// missing trailing component as 0 (so 3.12 == 3.12.0). Returns -1, 0, or 1. +func compareRelease(a, b []int) int { + n := max(len(a), len(b)) + for i := range n { + var ai, bi int + if i < len(a) { + ai = a[i] + } + if i < len(b) { + bi = b[i] + } + if ai != bi { + if ai < bi { + return -1 + } + return 1 + } + } + return 0 +} diff --git a/libs/localenv/warnings_test.go b/libs/localenv/warnings_test.go new file mode 100644 index 00000000000..c26fbba72a3 --- /dev/null +++ b/libs/localenv/warnings_test.go @@ -0,0 +1,114 @@ +package localenv + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// codes extracts the warning codes in order for concise assertions. +func codes(ws []Warning) []string { + out := make([]string, 0, len(ws)) + for _, w := range ws { + out = append(out, w.Code) + } + return out +} + +func TestDetectMergeWarningsGreenfieldAndEmpty(t *testing.T) { + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + // No pre-existing file: nothing of the user's to override. + assert.Nil(t, detectMergeWarnings(nil, c)) + assert.Nil(t, detectMergeWarnings([]byte{}, c)) + // Unparseable TOML is best-effort: no warnings, no panic. + assert.Nil(t, detectMergeWarnings([]byte("this is : not valid toml ["), c)) +} + +func TestDetectMergeWarningsOverriddenPins(t *testing.T) { + user := []byte(`[project] +name = "demo" +requires-python = ">=3.10" + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + got := detectMergeWarnings(user, c) + assert.Equal(t, []string{WarnRequiresPythonOverridden, WarnDBConnectPinOverridden}, codes(got)) +} + +func TestDetectMergeWarningsNoOverrideWhenMatching(t *testing.T) { + // User already matches the env pins exactly — the merge is a no-op, so no + // override warnings. + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=18.0.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect~=18.0.0"} + assert.Empty(t, detectMergeWarnings(user, c)) +} + +func TestDetectMergeWarningsConstraintsOnlyIgnoresDBConnect(t *testing.T) { + // In constraints-only mode c.DatabricksConnect is empty; the user's dev pin is + // left untouched, so it must not produce an override warning. + user := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = ["databricks-connect~=16.1.0"] +`) + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: ""} + assert.Empty(t, detectMergeWarnings(user, c)) +} + +func TestDetectMergeWarningsUserConstraintConflict(t *testing.T) { + user := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==17.0.0", "requests>=2.0"] +`) + // Env constrains pyarrow to ~=21.0.0 (admits [21.0.0, 21.1.0)); the user's + // ==17.0.0 is provably outside it. requests is not constrained → no conflict. + c := Constraints{ + RequiresPython: "==3.12.*", + ConstraintDeps: []string{"pyarrow~=21.0.0", "numpy~=2.1.3"}, + } + got := detectMergeWarnings(user, c) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(got)) + assert.Contains(t, got[0].Message, "pyarrow") +} + +func TestDetectMergeWarningsNoConflictWhenCompatible(t *testing.T) { + user := []byte(`[project] +requires-python = "==3.12.*" +dependencies = ["pyarrow==21.0.3"] +`) + // 21.0.3 is inside ~=21.0.0's [21.0, 22.0) range — compatible, no warning. + c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow~=21.0.0"}} + assert.Empty(t, detectMergeWarnings(user, c)) +} + +func TestRangesDisjoint(t *testing.T) { + cases := []struct { + user, env string + disjoint bool + why string + }{ + {"==17.0.0", "~=21.0.0", true, "exact pin below the compatible range"}, + {"==21.0.3", "~=21.0.0", false, "exact pin inside ~=21.0.0 == >=21.0.0,==21.0.*"}, + {"==21.5.0", "~=21.0.0", true, "~=21.0.0 fixes 21.0.*, so 21.5.0 is outside it"}, + {"==22.0.0", "~=21.0.0", true, "exact pin above the compatible upper bound"}, + {"==1.0", "==2.0", true, "two different exact pins"}, + {"==2.0", "==2.0", false, "identical exact pins"}, + {">=2.0", "==2.5", false, "cannot decide a lower-bound vs exact conservatively"}, + {"!=2.0", "==2.0", false, "!= is not modeled — never a conflict"}, + {">=2.0,<3.0", "==5.0", false, "multi-clause range is treated as unknown"}, + {"==2.*", "==2.0", false, "wildcards are unparsed — no conflict"}, + {"", "~=21.0.0", false, "no user spec — nothing to compare"}, + } + for _, tc := range cases { + assert.Equalf(t, tc.disjoint, rangesDisjoint(tc.user, tc.env), + "rangesDisjoint(%q,%q): %s", tc.user, tc.env, tc.why) + } +} From 7f283035af09754dcf8f1553068d1038da52d8cf Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Thu, 6 Aug 2026 10:48:30 +0200 Subject: [PATCH 2/3] localenv: harden merge-warning detection against real-world pyproject input MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review of the setup-local warnings work. Four defects, all in the advisory/telemetry path (no effect on the merged file or the run's outcome): - A PEP 735 {include-group = ...} entry in [dependency-groups].dev made the strict decode fail, and detectMergeWarnings returned no warnings at all — even though [project] had already parsed and MergeManaged still rewrote all three managed regions. The user's pins were replaced with no advisory, and the warning histogram under-counted exactly the projects using a standard uv feature. The user's file now decodes through userPyprojectTOML, which models dev as []any and skips non-string entries; the artifact keeps the strict struct since we control its shape. - parseClause accepted a single-segment "~=" (e.g. "~=2"), which PEP 440 does not define. compatibleReleaseContains returns "not contained" for such a base, and the disjointness callers read that as proof of disjointness — so overlapping ranges were reported as conflicts ("requests~=2" vs "requests==2.31.0"). Such a clause is now refused so it stays an unknown range, restoring the "only provably disjoint" invariant the code documents. - Duplicate constraint-dependencies entries for one package let the last one win, making the outcome depend on artifact ordering (["pyarrow<21","pyarrow>=20"] reported a conflict; the reverse order did not). Entries for a package compose as a conjunction, so they are now joined and treated as an unknown range. - Extras were left in the specifier, so "pyarrow[compute]==17.0.0" never parsed and a real conflict went unreported. Extras select optional features and never narrow the version range, so they are stripped. Requirements carrying an environment marker are now skipped instead of half-parsed, since we do not evaluate markers and the pin may not apply to the resolving interpreter. Also drops the [project].dependencies field added to pyprojectTOML, which the new user-side struct supersedes. Verified by brute-forcing 5184 ordered specifier pairs (including the malformed single-segment "~=" forms) against a dense version grid: every "disjoint" verdict is backed by the absence of a witness version satisfying both sides, and ~= semantics match PEP 440's worked examples via an independent interval model. Co-authored-by: Isaac --- libs/localenv/constraints.go | 3 +- libs/localenv/warnings.go | 101 +++++++++++++++++++++++++++------ libs/localenv/warnings_test.go | 73 +++++++++++++++++++++++- 3 files changed, 156 insertions(+), 21 deletions(-) diff --git a/libs/localenv/constraints.go b/libs/localenv/constraints.go index 08a6c311427..eae55263a6e 100644 --- a/libs/localenv/constraints.go +++ b/libs/localenv/constraints.go @@ -236,8 +236,7 @@ func fetchURL(ctx context.Context, url string) ([]byte, error) { // pyprojectTOML mirrors the pyproject.toml fields we care about. type pyprojectTOML struct { Project struct { - RequiresPython string `toml:"requires-python"` - Dependencies []string `toml:"dependencies"` + RequiresPython string `toml:"requires-python"` } `toml:"project"` DependencyGroups struct { Dev []string `toml:"dev"` diff --git a/libs/localenv/warnings.go b/libs/localenv/warnings.go index c7565a0ba25..fdaeda05d69 100644 --- a/libs/localenv/warnings.go +++ b/libs/localenv/warnings.go @@ -10,10 +10,10 @@ import ( "github.com/BurntSushi/toml" ) -// depSpecRe splits a dependency string into its package name and the trailing -// version specifier (e.g. "pyarrow~=19.0" -> "pyarrow", "~=19.0"). It stops the -// name at the first PEP 508 separator; extras/markers/urls after the version are -// out of scope for the conservative conflict check. +// depSpecRe splits a dependency string into its package name and the remainder +// (e.g. "pyarrow~=19.0" -> "pyarrow", "~=19.0"). It stops the name at the first +// PEP 508 separator; splitDepSpec handles the extras and markers that may lead the +// remainder. var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`) // singleClauseRe parses one version clause: an operator and a dotted numeric @@ -21,14 +21,60 @@ var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`) // cannot parse this simply is treated as "unknown" and never yields a conflict. var singleClauseRe = regexp.MustCompile(`^(>=|<=|==|~=|!=|<|>)?\s*([0-9]+(?:\.[0-9]+)*)`) -// splitDepSpec returns the normalized package name and the raw version specifier -// portion of a dependency string. ok is false when there is no recognizable name. +// splitDepSpec returns the normalized package name and the version specifier +// portion of a dependency string. ok is false when there is no recognizable name, +// or when the requirement carries an environment marker: a marker makes the +// dependency conditional on the resolving interpreter, which we do not evaluate, +// so comparing its range could flag a pin that never applies. func splitDepSpec(dep string) (name, spec string, ok bool) { - m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep)) + dep = strings.TrimSpace(dep) + if strings.Contains(dep, ";") { + return "", "", false + } + m := depSpecRe.FindStringSubmatch(dep) if m == nil { return "", "", false } - return normalizePackageName(m[1]), strings.TrimSpace(m[2]), true + // Extras select optional features and never narrow the version range, so drop + // them to expose the specifier underneath ("pkg[a,b]==1.0" -> "==1.0"). + spec = strings.TrimSpace(m[2]) + if strings.HasPrefix(spec, "[") { + if i := strings.Index(spec, "]"); i >= 0 { + spec = strings.TrimSpace(spec[i+1:]) + } + } + return normalizePackageName(m[1]), spec, true +} + +// userPyprojectTOML is a permissive view of the fields detectMergeWarnings reads +// from the *user's* pyproject.toml. +// +// It deliberately does not reuse pyprojectTOML: that struct decodes the +// Databricks-owned constraint artifact, whose shape we control, whereas a user's +// dependency group may legitimately hold PEP 735 table entries such as +// {include-group = "test"} alongside requirement strings. Decoding dev as []any +// (and declaring only the fields actually read) keeps one unrelated entry from +// failing the whole document and silently dropping every warning. +type userPyprojectTOML struct { + Project struct { + RequiresPython string `toml:"requires-python"` + Dependencies []string `toml:"dependencies"` + } `toml:"project"` + DependencyGroups struct { + Dev []any `toml:"dev"` + } `toml:"dependency-groups"` +} + +// devRequirements returns the requirement strings of a dependency group, skipping +// PEP 735 table entries (include-group) that are not requirements themselves. +func devRequirements(entries []any) []string { + out := make([]string, 0, len(entries)) + for _, e := range entries { + if s, ok := e.(string); ok { + out = append(out, s) + } + } + return out } // detectMergeWarnings compares a user's existing pyproject.toml against the @@ -37,16 +83,18 @@ func splitDepSpec(dep string) (name, spec string, ok bool) { // merge; MergeManaged still owns the actual byte edits. // // It is best-effort: unparseable input yields no warnings rather than an error, -// because a warning is advisory and must never fail the run. Greenfield projects -// (no pre-existing content) produce nothing — there is nothing of the user's to -// override. Warnings are deterministic and ordered (requires-python, then -// databricks-connect, then constraint conflicts sorted by package) so goldens are -// stable. +// because a warning is advisory and must never fail the run. The user's file is +// decoded through userPyprojectTOML rather than the artifact's stricter +// pyprojectTOML, so a legitimate PEP 735 table entry cannot suppress the checks +// that do not depend on it. Greenfield projects (no pre-existing content) produce +// nothing — there is nothing of the user's to override. Warnings are deterministic +// and ordered (requires-python, then databricks-connect, then constraint conflicts +// sorted by package) so goldens are stable. func detectMergeWarnings(userPyproject []byte, c Constraints) []Warning { if len(userPyproject) == 0 { return nil } - var p pyprojectTOML + var p userPyprojectTOML if err := toml.Unmarshal(userPyproject, &p); err != nil { return nil } @@ -66,7 +114,7 @@ func detectMergeWarnings(userPyproject []byte, c Constraints) []Warning { // the merge replaces it. Only meaningful in default mode (c.DatabricksConnect // is empty in constraints-only, where the dev group is left untouched). if c.DatabricksConnect != "" { - for _, entry := range p.DependencyGroups.Dev { + for _, entry := range devRequirements(p.DependencyGroups.Dev) { if !isDatabricksConnectDep(entry) { continue } @@ -94,12 +142,21 @@ func constraintConflicts(userDeps, envConstraints []string) []Warning { if len(userDeps) == 0 || len(envConstraints) == 0 { return nil } - // Index the env constraints by normalized package name. + // Index the env constraints by normalized package name. Multiple entries for one + // package compose as a conjunction, so they are joined with "," rather than + // letting the last one win: parseClause treats a multi-clause spec as an unknown + // range, which keeps the outcome independent of artifact ordering instead of + // deciding against an arbitrary subset of the clauses. envByName := make(map[string]string, len(envConstraints)) for _, ec := range envConstraints { - if name, spec, ok := splitDepSpec(ec); ok && spec != "" { - envByName[name] = spec + name, spec, ok := splitDepSpec(ec) + if !ok || spec == "" { + continue } + if prev, dup := envByName[name]; dup { + spec = prev + "," + spec + } + envByName[name] = spec } if len(envByName) == 0 { return nil @@ -169,6 +226,14 @@ func parseClause(spec string) (clause, bool) { } rel = append(rel, n) } + // PEP 440 requires at least two release segments after "~=" (it expands to + // ">=X.Y, ==X.*", which needs a segment to hold fixed). A single-segment base + // has no defined range, and compatibleReleaseContains reports "not contained" + // for it — which the disjointness callers would read as a proof of disjointness + // and report a conflict. Refuse to parse it so it stays an unknown range. + if op == "~=" && len(rel) < 2 { + return clause{}, false + } return clause{op: op, rel: rel}, true } diff --git a/libs/localenv/warnings_test.go b/libs/localenv/warnings_test.go index c26fbba72a3..2062832ec44 100644 --- a/libs/localenv/warnings_test.go +++ b/libs/localenv/warnings_test.go @@ -84,11 +84,74 @@ func TestDetectMergeWarningsNoConflictWhenCompatible(t *testing.T) { requires-python = "==3.12.*" dependencies = ["pyarrow==21.0.3"] `) - // 21.0.3 is inside ~=21.0.0's [21.0, 22.0) range — compatible, no warning. + // 21.0.3 is inside ~=21.0.0's [21.0.0, 21.1.0) range — compatible, no warning. c := Constraints{RequiresPython: "==3.12.*", ConstraintDeps: []string{"pyarrow~=21.0.0"}} assert.Empty(t, detectMergeWarnings(user, c)) } +func TestDetectMergeWarningsIncludeGroupDoesNotSuppress(t *testing.T) { + // A PEP 735 {include-group = ...} table is a legal dev-group entry that uv + // supports. It must not fail the decode and take every unrelated warning with + // it: the requires-python override and the pyarrow conflict below do not depend + // on the dev group at all, and MergeManaged rewrites all three regions anyway. + user := []byte(`[project] +requires-python = ">=3.9" +dependencies = ["pyarrow==19.0.0"] + +[dependency-groups] +dev = ["databricks-connect~=16.1.0", {include-group = "test"}] +test = [] +`) + c := Constraints{ + RequiresPython: "==3.12.*", + DatabricksConnect: "databricks-connect~=17.2.0", + ConstraintDeps: []string{"pyarrow~=21.0.0"}, + } + assert.Equal(t, []string{ + WarnRequiresPythonOverridden, + WarnDBConnectPinOverridden, + WarnUserConstraintConflict, + }, codes(detectMergeWarnings(user, c))) +} + +func TestConstraintConflictsDuplicateEnvEntriesAreOrderIndependent(t *testing.T) { + // Constraint entries for one package compose as a conjunction. Joining them + // yields a multi-clause spec, which is an unknown range — so neither ordering + // decides a conflict against just one of the clauses. + user := []string{"pyarrow==19.0.0"} + assert.Empty(t, constraintConflicts(user, []string{"pyarrow<21", "pyarrow>=20"})) + assert.Empty(t, constraintConflicts(user, []string{"pyarrow>=20", "pyarrow<21"})) +} + +func TestSplitDepSpecExtrasAndMarkers(t *testing.T) { + // Extras never narrow the version range, so they are stripped and the pin + // underneath is still compared. + name, spec, ok := splitDepSpec("pyarrow[compute,parquet]==17.0.0") + assert.True(t, ok) + assert.Equal(t, "pyarrow", name) + assert.Equal(t, "==17.0.0", spec) + + // A marker makes the requirement conditional on the resolving interpreter, + // which we do not evaluate — so it is skipped rather than compared. + _, _, ok = splitDepSpec(`pyarrow==17.0.0; python_version < "3.12"`) + assert.False(t, ok) + + // PEP 503 normalization applies to the name on both sides of the lookup. + name, spec, ok = splitDepSpec("PyArrow_Extra ==1.0") + assert.True(t, ok) + assert.Equal(t, "pyarrow-extra", name) + assert.Equal(t, "==1.0", spec) +} + +func TestDetectMergeWarningsConflictThroughExtras(t *testing.T) { + user := []byte(`[project] +dependencies = ["pyarrow[compute]==17.0.0"] +`) + c := Constraints{ConstraintDeps: []string{"pyarrow~=21.0.0"}} + got := detectMergeWarnings(user, c) + assert.Equal(t, []string{WarnUserConstraintConflict}, codes(got)) +} + func TestRangesDisjoint(t *testing.T) { cases := []struct { user, env string @@ -106,6 +169,14 @@ func TestRangesDisjoint(t *testing.T) { {">=2.0,<3.0", "==5.0", false, "multi-clause range is treated as unknown"}, {"==2.*", "==2.0", false, "wildcards are unparsed — no conflict"}, {"", "~=21.0.0", false, "no user spec — nothing to compare"}, + // PEP 440 requires two release segments after "~=", so a single-segment base + // has no defined range. It must stay undecidable rather than being read as a + // proof of disjointness: 2.0 and 2.31.0 both plainly satisfy any reading of ~=2. + {"~=2", "==2.0", false, "single-segment ~= is malformed — undecidable, not disjoint"}, + {"==2.0", "~=2", false, "same, with the malformed base on the env side"}, + {"~=2", "==2.31.0", false, "single-segment ~= never decides a conflict"}, + {"~=2.0", "==2.31.0", false, "valid ~=2.0 admits 2.31.0 (>=2.0, ==2.*)"}, + {"~=2.1", "==2.0.5", true, "valid ~=2.1 excludes 2.0.5 (below the floor)"}, } for _, tc := range cases { assert.Equalf(t, tc.disjoint, rangesDisjoint(tc.user, tc.env), From 9d6bd19536a8a44b8aba93d14d299ad2a83786df Mon Sep 17 00:00:00 2001 From: Grigory Panov Date: Thu, 6 Aug 2026 12:04:38 +0200 Subject: [PATCH 3/3] localenv: follow include-group when scanning for the user's databricks-connect pin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- libs/localenv/warnings.go | 75 +++++++++++++++++++++++++--------- libs/localenv/warnings_test.go | 61 ++++++++++++++++++++++++++- 2 files changed, 115 insertions(+), 21 deletions(-) diff --git a/libs/localenv/warnings.go b/libs/localenv/warnings.go index fdaeda05d69..17b1b868305 100644 --- a/libs/localenv/warnings.go +++ b/libs/localenv/warnings.go @@ -22,16 +22,11 @@ var depSpecRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)\s*(.*)$`) var singleClauseRe = regexp.MustCompile(`^(>=|<=|==|~=|!=|<|>)?\s*([0-9]+(?:\.[0-9]+)*)`) // splitDepSpec returns the normalized package name and the version specifier -// portion of a dependency string. ok is false when there is no recognizable name, -// or when the requirement carries an environment marker: a marker makes the -// dependency conditional on the resolving interpreter, which we do not evaluate, -// so comparing its range could flag a pin that never applies. +// portion of a dependency string, with any extras stripped. ok is false when there +// is no recognizable name, or when what follows the name carries an environment +// marker (see below). func splitDepSpec(dep string) (name, spec string, ok bool) { - dep = strings.TrimSpace(dep) - if strings.Contains(dep, ";") { - return "", "", false - } - m := depSpecRe.FindStringSubmatch(dep) + m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep)) if m == nil { return "", "", false } @@ -43,6 +38,15 @@ func splitDepSpec(dep string) (name, spec string, ok bool) { spec = strings.TrimSpace(spec[i+1:]) } } + // A marker gates the whole requirement on the resolving interpreter, which we do + // not evaluate, so a pin that may not apply must not be compared. Checking the + // remainder rather than the raw string scopes this to where a marker can actually + // appear, leaving an extras list that happens to contain ";" alone. A ";" inside + // a url requirement still lands here, which costs nothing: a url has no + // comparable version range and would be undecidable regardless. + if strings.Contains(spec, ";") { + return "", "", false + } return normalizePackageName(m[1]), spec, true } @@ -60,20 +64,51 @@ type userPyprojectTOML struct { RequiresPython string `toml:"requires-python"` Dependencies []string `toml:"dependencies"` } `toml:"project"` - DependencyGroups struct { - Dev []any `toml:"dev"` - } `toml:"dependency-groups"` + // Every group is decoded, not just dev, because a dev entry may pull in another + // group by reference — see groupRequirements. + DependencyGroups map[string][]any `toml:"dependency-groups"` } -// devRequirements returns the requirement strings of a dependency group, skipping -// PEP 735 table entries (include-group) that are not requirements themselves. -func devRequirements(entries []any) []string { - out := make([]string, 0, len(entries)) - for _, e := range entries { - if s, ok := e.(string); ok { - out = append(out, s) +// devGroup is the dependency group whose databricks-connect pin the merge manages. +const devGroup = "dev" + +// groupRequirements returns the requirement strings reachable from the named +// dependency group, following PEP 735 {include-group = "..."} indirections. +// +// Resolving the indirection matters because MergeManaged writes the env's pin into +// dev regardless of where the user's own pin lives: with dev = [{include-group = +// "spark"}] and the pin inside the spark group, a non-recursive scan reports no +// override while the merged file ends up carrying two pins for the same package. +// Group names are compared under PEP 503 normalization, which PEP 735 also +// specifies for group names. A group already visited is skipped, so an +// include-group cycle terminates instead of recursing forever. +func groupRequirements(groups map[string][]any, name string) []string { + byName := make(map[string][]any, len(groups)) + for g, entries := range groups { + byName[normalizePackageName(g)] = entries + } + + var out []string + visited := make(map[string]bool, len(groups)) + var walk func(string) + walk = func(g string) { + g = normalizePackageName(g) + if visited[g] { + return + } + visited[g] = true + for _, e := range byName[g] { + switch v := e.(type) { + case string: + out = append(out, v) + case map[string]any: + if inc, ok := v["include-group"].(string); ok { + walk(inc) + } + } } } + walk(name) return out } @@ -114,7 +149,7 @@ func detectMergeWarnings(userPyproject []byte, c Constraints) []Warning { // the merge replaces it. Only meaningful in default mode (c.DatabricksConnect // is empty in constraints-only, where the dev group is left untouched). if c.DatabricksConnect != "" { - for _, entry := range devRequirements(p.DependencyGroups.Dev) { + for _, entry := range groupRequirements(p.DependencyGroups, devGroup) { if !isDatabricksConnectDep(entry) { continue } diff --git a/libs/localenv/warnings_test.go b/libs/localenv/warnings_test.go index 2062832ec44..e5a5db5af59 100644 --- a/libs/localenv/warnings_test.go +++ b/libs/localenv/warnings_test.go @@ -114,6 +114,62 @@ test = [] }, codes(detectMergeWarnings(user, c))) } +func TestDetectMergeWarningsIncludeGroupIndirection(t *testing.T) { + // The user's pin is reachable only through an include-group, but MergeManaged + // writes the env's pin into dev anyway — so without following the indirection the + // merged file would carry two databricks-connect pins and no override warning. + c := Constraints{RequiresPython: "==3.12.*", DatabricksConnect: "databricks-connect==17.0.0"} + + indirect := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(indirect, c))) + + // An include-group cycle must terminate rather than recurse forever, and the pin + // behind it is still found. + cyclic := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "a"}] +a = [{include-group = "dev"}, "databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(cyclic, c))) + + // PEP 735 normalizes group names the same way PEP 503 normalizes package names. + renamed := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "My_Spark.Group"}] +"my-spark-group" = ["databricks-connect==16.1.0"] +`) + assert.Equal(t, []string{WarnDBConnectPinOverridden}, codes(detectMergeWarnings(renamed, c))) + + // An included pin that already matches the env is not an override. + matching := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "spark"}] +spark = ["databricks-connect==17.0.0"] +`) + assert.Empty(t, detectMergeWarnings(matching, c)) + + // A dangling reference is not an error; there is simply no pin to compare. + dangling := []byte(`[project] +requires-python = "==3.12.*" + +[dependency-groups] +dev = [{include-group = "absent"}] +`) + assert.Empty(t, detectMergeWarnings(dangling, c)) +} + func TestConstraintConflictsDuplicateEnvEntriesAreOrderIndependent(t *testing.T) { // Constraint entries for one package compose as a conjunction. Joining them // yields a multi-clause spec, which is an unknown range — so neither ordering @@ -132,9 +188,12 @@ func TestSplitDepSpecExtrasAndMarkers(t *testing.T) { assert.Equal(t, "==17.0.0", spec) // A marker makes the requirement conditional on the resolving interpreter, - // which we do not evaluate — so it is skipped rather than compared. + // which we do not evaluate — so it is skipped rather than compared, with or + // without extras in front of it. _, _, ok = splitDepSpec(`pyarrow==17.0.0; python_version < "3.12"`) assert.False(t, ok) + _, _, ok = splitDepSpec(`pyarrow[compute]==17.0.0; python_version < "3.12"`) + assert.False(t, ok) // PEP 503 normalization applies to the name on both sides of the lookup. name, spec, ok = splitDepSpec("PyArrow_Extra ==1.0")