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/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..17b1b868305 --- /dev/null +++ b/libs/localenv/warnings.go @@ -0,0 +1,374 @@ +package localenv + +import ( + "fmt" + "regexp" + "slices" + "strconv" + "strings" + + "github.com/BurntSushi/toml" +) + +// 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 +// 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 version specifier +// 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) { + m := depSpecRe.FindStringSubmatch(strings.TrimSpace(dep)) + if m == nil { + return "", "", false + } + // 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:]) + } + } + // 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 +} + +// 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"` + // 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"` +} + +// 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 +} + +// 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. 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 userPyprojectTOML + 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 groupRequirements(p.DependencyGroups, devGroup) { + 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. 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 { + 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 + } + + 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) + } + // 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 +} + +// 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..e5a5db5af59 --- /dev/null +++ b/libs/localenv/warnings_test.go @@ -0,0 +1,244 @@ +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.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 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 + // 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, 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") + 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 + 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"}, + // 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), + "rangesDisjoint(%q,%q): %s", tc.user, tc.env, tc.why) + } +}