Skip to content

fix(langmgr): append +incompatible suffix for pre-module major>=2 Go deps - #1682

Merged
robert-cronin merged 2 commits into
mainfrom
fix/go-incompatible-suffix
Aug 28, 2026
Merged

fix(langmgr): append +incompatible suffix for pre-module major>=2 Go deps#1682
robert-cronin merged 2 commits into
mainfrom
fix/go-incompatible-suffix

Conversation

@omercnet

Copy link
Copy Markdown
Contributor

Problem

go get fails outright when patching a pre-modules-era Go dependency released at major version 2 or higher whose module path carries no /vN suffix. The canonical case is github.com/docker/docker@v28.x: the module path is not modules-aware, so the Go toolchain only accepts the version with the +incompatible build tag. Scanner reports routinely report the bare version (v28.0.0), and the patch run dies on the go get step.

Root cause

pkg/langmgr/golang.go builds the go get spec string verbatim from the report's fixed version in two places, updateGoModule and upgradePackagesWithTooling. Both only normalize the v prefix, so a bare v28.0.0 is passed through unchanged.

Fix

New helper appendIncompatibleIfNeeded(modulePath, version) applied at both spec construction sites, right after the v-prefix normalization. It appends +incompatible only when all of the following hold:

  • the version is valid semver (non-semver input is returned untouched)
  • it is not already suffixed
  • the major is not v0 or v1
  • module.SplitPathVersion reports no path major suffix, meaning the dependency is not modules-aware

Everything else is returned unchanged, so /v2-style modules, v0/v1 deps, and pseudo-versions below v2 keep their current behavior.

Tests

  • TestAppendIncompatibleIfNeeded: table-driven coverage for github.com/docker/docker@v28.0.0, /v2 path suffix, v0 and v1 majors, already-suffixed input, pseudo-versions at v0 and at v2 with and without a path suffix, and invalid/empty version input.
  • TestIncompatibleVersionsPassValidation: confirms isValidGoVersion, validateGoVersion, and cleanGoVersion all accept a +incompatible version (semver treats it as build metadata, and + is not in the shell-unsafe character set).

go build ./..., go test ./pkg/langmgr, and gofumpt all pass; make lint reports only pre-existing findings unrelated to this change.

@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.84%. Comparing base (2a7738b) to head (7b5b429).

Files with missing lines Patch % Lines
pkg/langmgr/golang.go 88.88% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1682      +/-   ##
==========================================
+ Coverage   53.74%   53.84%   +0.10%     
==========================================
  Files          69       69              
  Lines       14152    14169      +17     
==========================================
+ Hits         7606     7630      +24     
+ Misses       5802     5795       -7     
  Partials      744      744              
Files with missing lines Coverage Δ
pkg/langmgr/golang.go 24.27% <88.88%> (+3.28%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@omercnet
omercnet force-pushed the fix/go-incompatible-suffix branch from 72b53c9 to 008a198 Compare August 25, 2026 06:27
Comment thread pkg/langmgr/golang.go
Comment thread pkg/langmgr/golang.go
@omercnet
omercnet force-pushed the fix/go-incompatible-suffix branch from 008a198 to cb82a16 Compare August 26, 2026 10:26
@omercnet

Copy link
Copy Markdown
Contributor Author

Review feedback addressed in cb82a16 (rebased on current main):

  • The binary-rebuild path now builds its update map through buildBinaryUpdateMap, which applies the v prefix and appendIncompatibleIfNeeded before the versions become go.mod requirements, so it no longer records bare pre-modules v2+ requirements.
  • appendIncompatibleIfNeeded now short-circuits on any existing build metadata (strings.Contains(version, "+")) rather than only a literal +incompatible suffix, so v2.0.0+build1 can never grow a second + component.
  • Regression coverage: TestBuildBinaryUpdateMap and a new v2.0.0+build1 case in TestAppendIncompatibleIfNeeded.

CI: 74 of 76 checks pass. The two failures, lint and Test Bulk Patching, are inherited from main and unrelated to this diff, which only touches pkg/langmgr. Both come from integration/bulk/singlearch_patch_test.go failing typecheck after the testcontainers 0.44.0 bump (docker/docker vs moby/moby container types); the same two checks fail on main's head. #1685 fixes that, and this PR should go green once it lands.

robert-cronin
robert-cronin previously approved these changes Aug 27, 2026

@robert-cronin robert-cronin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both findings are resolved; could you rebase onto current main so the #1683 tests and this PR's tests are both retained and CI reruns?

…deps

go get rejects a bare version for dependencies released at major version 2
or higher whose module path has no /vN suffix (github.com/docker/docker@v28.x
being the canonical case); such versions must carry the +incompatible build
tag. Scanner reports frequently omit it, so the spec strings Copa builds for
go get failed outright.

Normalize the version after the v-prefix fix in both places a go get spec is
constructed, using module.SplitPathVersion to detect a path major suffix and
leaving v0/v1 majors, already-suffixed versions, and non-semver input alone.

Signed-off-by: Omer Cohen <639682+omercnet@users.noreply.github.com>
Normalize the binary-rebuild update map through the same version handling
the in-image `go get` path uses, so go.mod requirements written during a
binary rebuild no longer record bare pre-modules major>=2 versions.

Leave versions that already carry build metadata untouched: semver permits
a single '+' component, so appending the tag to v2.0.0+build1 produced an
invalid v2.0.0+build1+incompatible.

Signed-off-by: Omer Cohen <639682+omercnet@users.noreply.github.com>
@omercnet

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (2a7738b, which includes #1683) and force-pushed as 7b5b429. Note this dismissed the approval, so a re-approve is needed once CI settles.

The conflict was in pkg/langmgr/golang_test.go, where both branches appended test functions at the same spot. Both sets are retained in full: TestFilterGoDowngrades and TestGolangManagerInstallUpdatesSkipsNonNewerVersion from #1683, plus TestAppendIncompatibleIfNeeded, TestIncompatibleVersionsPassValidation and TestBuildBinaryUpdateMap from this PR. All five pass, and the diff against main is still confined to pkg/langmgr with no deletions from #1683.

@omercnet
omercnet requested a review from robert-cronin August 27, 2026 13:24
@robert-cronin
robert-cronin merged commit bce4922 into main Aug 28, 2026
79 checks passed
@robert-cronin
robert-cronin deleted the fix/go-incompatible-suffix branch August 28, 2026 01:02
@github-project-automation github-project-automation Bot moved this from 🆕 New to ✅ Done in Copacetic Workboard Aug 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

2 participants