Trim potential GOEXPERIMENT flag in buildinfo #197
Conversation
WalkthroughUpdate to Changes
Sequence Diagram(s)(Skipped — changes are localized parsing/normalization tweaks and do not introduce new control-flow interactions.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/goutil/goutil.go (1)
390-406: Don’t return the GOEXPERIMENT suffix; it breaks version parsing and up‑to‑date checks.Including " X:…" in the returned string causes versionUpToDate(...) to feed values like "1.22.3 X:nodwarf5" to hashicorp/go-version, which fails to parse and makes tools appear out-of-date. Capture the suffix but exclude it from the returned value.
Apply this diff to keep matching the suffix while returning only the base "go..." token:
-var goVersionRegex = regexp.MustCompile(`(^|\s)(go[1-9]\S+( X:\S+)?)`) +// capture 1: boundary, capture 2: base go version token, non-capturing optional GOEXPERIMENT tail +var goVersionRegex = regexp.MustCompile(`(^|\s)(go[1-9]\S+)(?:\sX:\S+)?`)No change needed below;
GetInstalledGoVersionshould continue to returnm[2], which will now be the base version only.
🧹 Nitpick comments (2)
internal/goutil/goutil.go (2)
133-139: TrimPrefix instead of TrimLeft for version prefixes.TrimLeft("go") removes any leading 'g'/'o' runes, not the exact "go" prefix. Same for "v". Use TrimPrefix to avoid accidental stripping.
Example:
strings.TrimPrefix(p.Version.Current, "v") strings.TrimPrefix(p.Version.Latest, "v") strings.TrimPrefix(p.GoVersion.Current, "go") strings.TrimPrefix(p.GoVersion.Latest, "go")
392-409: Consider normalizing buildinfo.GoVersion too (may also include " X:…").If info.GoVersion ever carries the GOEXPERIMENT tail, comparisons will still fail. Optionally normalize both sides.
Minimal helper (can live in this file):
func normalizeGoVersion(s string) string { // keep leading "go" for display elsewhere; strip for semver parsing as needed if i := strings.Index(s, " X:"); i >= 0 { s = s[:i] } return strings.TrimSpace(s) }Use when setting
pkg.GoVersion.Currentand the value returned fromGetInstalledGoVersion()before comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/goutil/goutil.go(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
internal/goutil/goutil.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/goutil/goutil.go (1)
internal/cmdinfo/cmdinfo.go (1)
Version(9-9)
🔇 Additional comments (1)
internal/goutil/goutil.go (1)
134-139: Good switch to TrimPrefix; avoids over-stripping.Replacing TrimLeft with TrimPrefix prevents accidentally removing multiple leading characters. This is correct for both "v" and "go" prefixes.
mochaaP
changed the title
|
@mochaaP |
2 participants
On my Fedora system, Go is built with GOEXPERIMENT=nodwarf5, so GoVersion includes X:nodwarf5 in the end. https://github.com/golang/go/blob/80038586ed2814a03dcb95cd6f130766f8d803c3/src/runtime/extern.go#L368-L369
Summary by CodeRabbit
Bug Fixes
Chores