feat: overhaul #4
Conversation
james-d-elliott
force-pushed
the
feat-overhaul
branch
from
923936b to
4ef8afa
Compare
james-d-elliott
force-pushed
the
feat-overhaul
branch
from
3ab186d to
5d11e26
Compare
Signed-off-by: James Elliott <[email protected]>
Signed-off-by: James Elliott <[email protected]>
Signed-off-by: James Elliott <[email protected]>
Signed-off-by: James Elliott <[email protected]>
james-d-elliott
force-pushed
the
feat-overhaul
branch
from
b13c79e to
a9fdb9c
Compare
Signed-off-by: James Elliott <[email protected]>
WalkthroughThis pull request introduces new modules and refactors existing code in the Go build system. New files add constant definitions, structured version constraints, and a comprehensive compilation options mechanism. Legacy and updated version-retrieval functions have been provided. Core files such as those responsible for flag handling, platform determination, and toolchain building have been simplified and updated to work with a structured version type from the go-version package. New tests ensure that the build argument generation and platform mappings work as expected. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant M as Main (main.go)
participant O as CompileOpts (opts.go)
participant T as Toolchain (toolchain.go)
U->>M: Provide build flags/arguments
M->>O: Initialize CompileOpts based on flags
O->>O: Generate build arguments & environment (Arguments() & Env())
M->>T: Call mainBuildToolchain with *version.Version and options
T->>T: Process build using updated constants and platform constraints
T->>U: Return build result
sequenceDiagram
participant V as GoVersion()
participant TD as Temp Directory
participant FS as Filesystem
participant Exec as "go run" Execution
V->>TD: Create temporary directory
V->>FS: Write temporary version.go source file
V->>Exec: Execute "go run version.go"
Exec-->>V: Return Go version output
V->>Caller: Return version string
✨ Finishing Touches
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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: 1
🧹 Nitpick comments (12)
version_legacy.go (1)
34-38: Consider moving stdout printing to caller.The function both returns the version and prints it to stdout. Consider removing the
fmt.Printfline to maintain separation of concerns - utility functions should typically focus on returning values rather than producing side effects.- fmt.Printf("Detected Go Version: %s\n", version)version.go (2)
33-37: Consider moving stdout printing to caller.The function both returns the version and prints it to stdout. Consider removing the
fmt.Printfline to maintain separation of concerns - utility functions should typically focus on returning values rather than producing side effects.- fmt.Printf("Detected Go Version: %s\n", version)
15-38: Code duplication between version implementation files.The implementations in
version.goandversion_legacy.goare nearly identical, differing only in the specific API calls. Consider extracting the common logic to a shared helper function to reduce duplication.platforms_output_test.go (1)
8-8: Consider adding a comment about code generation potentialSince this is a substantial amount of data that may need maintenance as new Go versions are released, it might be worth adding a more detailed comment about the code generation opportunity mentioned briefly on line 8.
// It would be useful at some stage to leverage this for a code generator for versions above 1.7. +// TODO: Implement a code generator that automatically updates this map by running `go tool dist list` for each new Go versionplatform_test.go (1)
178-180: Consider using direct version comparisonSince you're already using the version package for comparing versions, you could simplify this code slightly.
sort.Slice(versions, func(i, j int) bool { - return version.Must(version.NewSemver(versions[i])).LessThan(version.Must(version.NewSemver(versions[j]))) + v1, _ := version.NewVersion(versions[i]) + v2, _ := version.NewVersion(versions[j]) + return v1.LessThan(v2) })This avoids using
Mustwhich would panic on invalid versions and uses the sameNewVersionfunction used elsewhere in the code.main.go (4)
23-33: Large var block for flags is acceptable, but consider grouping for readability.Defining multiple flags in one var block is valid, although separating booleans and strings could increase legibility.
var ( - flagBuildToolchain, flagVerbose, flagCgo, flagTrimPath, flagListOSArch bool - flagGoCmd, flagLDFlags, flagTags string - flagOutput string - flagParallel int - flagPlatform PlatformFlag - - flagChangeDir, flagCoverPkg, flagASMFlags, flagBuildMode, flagBuildVCS, flagCompiler, flagGcCGOFlags, flagGcFlags string - flagInstallSuffix, flagMod, flagModFile, flagOverlay, flagProfileGuidedOptimization, flagPackageDir string - flagRebuild, flagRace, flagMSAN, flagASAN, flagCover, flagLinkShared, flagModCacheRW bool + // Booleans + flagBuildToolchain bool + flagVerbose bool + flagCgo bool + flagTrimPath bool + flagListOSArch bool + flagRebuild bool + flagRace bool + flagMSAN bool + flagASAN bool + flagCover bool + flagLinkShared bool + flagModCacheRW bool + // Strings + flagGoCmd string + flagLDFlags string + flagTags string + flagOutput string + flagPlatform PlatformFlag + flagChangeDir string + flagCoverPkg string + flagASMFlags string + flagBuildMode string + flagBuildVCS string + flagCompiler string + flagGcCGOFlags string + flagGcFlags string + flagInstallSuffix string + flagMod string + flagModFile string + flagOverlay string + flagProfileGuidedOptimization string + flagPackageDir string + + // Integers + flagParallel int )
39-45: Consider adding descriptive usage messages for these newly introduced flags.Several flags (e.g.,
"output","parallel","verbose") have empty usage text, making them less clear to end-users.flags.BoolVar(&flagListOSArch, "osarch-list", false, "") -flags.StringVar(&flagOutput, "output", "{{.Dir}}_{{.OS}}_{{.Arch}}", "") -flags.IntVar(&flagParallel, "parallel", -1, "") -flags.BoolVar(&flagVerbose, "verbose", false, "") -flags.StringVar(&flagGoCmd, "gocmd", gobin, "") +flags.StringVar(&flagOutput, "output", "{{.Dir}}_{{.OS}}_{{.Arch}}", "Specify output path template.") +flags.IntVar(&flagParallel, "parallel", -1, "Set the number of parallel builds.") +flags.BoolVar(&flagVerbose, "verbose", false, "Enable verbose build logs.") +flags.StringVar(&flagGoCmd, "gocmd", gobin, "Custom Go command path.")
88-103: Possible confusion in defaulting to 3 for Solaris systems.When
runtime.NumCPU() < 2on Solaris, you override it to 3. This is presumably intentional for Joyent containers, but it might surprise those with minimal CPU systems.
213-215: Dynamic environment overrides are a useful extension.Allows platform-specific argument adjustments with minimal duplication.
Consider adding a debug log to confirm when these overrides are applied.
opts.go (2)
10-48:CompileOptsstruct is well-organized but watch for mismatched comments.All fields nicely correlate to build flags. However, notice the comment for
BuildVCSsays// -buildvsinstead of// -buildvcs.- BuildVCS string // -buildvs + BuildVCS string // -buildvcs
50-60: Potential duplication risk when appending "coverageredesign".If
GOEXPERIMENTalready contains"coverageredesign", it may be appended again.You could avoid duplicates by checking if it already exists before appending.
const.go (1)
32-50: GOOS constants appear comprehensive, including older and lesser-used OS targets.Be mindful of deprecated or unsupported OS values (e.g.,
nacl) in future Go releases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
.github/workflows/test.ymlis excluded by!**/*.ymlgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (14)
const.go(1 hunks)constraints.go(1 hunks)go.go(5 hunks)main.go(6 hunks)main_osarch.go(1 hunks)meta.go(1 hunks)opts.go(1 hunks)opts_test.go(1 hunks)platform.go(3 hunks)platform_test.go(3 hunks)platforms_output_test.go(1 hunks)toolchain.go(3 hunks)version.go(1 hunks)version_legacy.go(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Go 1.10.8 test
opts_test.go
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:
🪛 GitHub Check: Go 1.9.7 test
opts_test.go
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:
🪛 GitHub Check: Go 1.8.7 test
opts_test.go
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:
🪛 GitHub Check: Go 1.7.6 test
opts_test.go
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:
🪛 GitHub Actions: build
opts_test.go
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsASMFlags failed: expected: []string{"-asmflags="foo""}, actual: []string{"-asmflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsGcCGOFlags failed: expected: []string{"-gccgoflags="foo""}, actual: []string{"-gccgoflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsGcFlags failed: expected: []string{"-gcflags="foo""}, actual: []string{"-gcflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsLDFlags failed: expected: []string{"-ldflags="foo""}, actual: []string{"-ldflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsTags failed: expected: []string{"-tags="foo""}, actual: []string{"-tags", "foo"}
🔇 Additional comments (69)
meta.go (1)
1-3: LGTM: Clean addition of version constant.The constant
metaVersiondefines the application's version, following Go naming conventions. Consider adding a brief documentation comment to explain its purpose and usage.version_legacy.go (3)
1-3: Good use of build constraints.Appropriately restricted to Go versions before 1.16, using both the new
//go:builddirective and the legacy// +buildcomment format for maximum compatibility.
21-25: Use proper package for Go <1.16.Using
ioutil.TempDiris correct for versions before Go 1.16 sinceos.MkdirTempwasn't available yet.
29-31: Use of appropriate legacy API.Correctly using
ioutil.WriteFilefor Go versions before 1.16.version.go (3)
1-3: Good use of build constraints.Appropriately restricted to Go 1.16 and newer versions, using both the new
//go:builddirective and the legacy// +buildcomment format for maximum compatibility.
20-25: Use of modern API for Go ≥1.16.Correctly using
os.MkdirTempwhich replaced the deprecatedioutil.TempDirin Go 1.16.
28-30: Use of modern API for Go ≥1.16.Correctly using
os.WriteFilewhich replaced the deprecatedioutil.WriteFilein Go 1.16.main_osarch.go (3)
5-7: Adding version package import.Appropriately adding the import for the go-version package to support the signature change in the function below.
16-18:✅ Verification successful
Verify SupportedPlatforms function signature.
The function passes the version directly to
SupportedPlatforms(). Ensure that function has also been updated to accept a*version.Versionparameter.
🏁 Script executed:
#!/bin/bash # Check the SupportedPlatforms function signature echo "Checking SupportedPlatforms function signature..." rg --no-heading -A 1 "func SupportedPlatforms\("Length of output: 255
Verified: SupportedPlatforms Function Signature is Correct
The
SupportedPlatformsfunction inplatform.goaccepts a*version.Versionparameter as expected, matching the usage inmain_osarch.go. No changes are required.
9-16:✅ Verification successful
Type-safety improvement in function signature.
The parameter type change from
stringto*version.Versionimproves type safety and enables more sophisticated version checking. Ensure all call sites have been updated to pass the correct type.
🏁 Script executed:
#!/bin/bash # Check for all call sites of mainListOSArch to ensure they've been updated echo "Searching for mainListOSArch call sites..." rg --no-heading -A 2 -B 2 "mainListOSArch\(" | grep -v "^main_osarch.go"Length of output: 265
Verified: All Call Sites Updated Correctly
The parameter change from
stringto*version.Versionhas been confirmed. The call site inmain.go(i.e.,return mainListOSArch(v)) correctly passes a version pointer, ensuring the improved type safety works as intended. No further changes are necessary.opts_test.go (2)
1-52: Thorough and well-structured test coverageThe test cases cover a wide range of compile options and version compatibility scenarios. This comprehensive approach ensures that the
Arguments()method works correctly across different configurations.🧰 Tools
🪛 GitHub Check: Go 1.10.8 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.9.7 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.8.7 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.7.6 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Actions: build
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsASMFlags failed: expected: []string{"-asmflags="foo""}, actual: []string{"-asmflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsGcCGOFlags failed: expected: []string{"-gccgoflags="foo""}, actual: []string{"-gccgoflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsGcFlags failed: expected: []string{"-gcflags="foo""}, actual: []string{"-gcflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsLDFlags failed: expected: []string{"-ldflags="foo""}, actual: []string{"-ldflags", "foo"}
[error] 48-48: TestCompileOpts_Arguments/ShouldReturnStringsTags failed: expected: []string{"-tags="foo""}, actual: []string{"-tags", "foo"}
4-4:❓ Verification inconclusive
Consider using constraints.go for package dependencies
The static analysis tools are failing to find the
github.com/stretchr/testify/assertpackage in older Go versions (1.7.6, 1.8.7, 1.9.7, 1.10.8).This may indicate that the testify package isn't compatible with those older versions. You may want to:
- Add a version constraint in your go.mod
- Update the CI configuration to skip tests on older Go versions
- Add a build constraint to skip this test file on older Go versions
🏁 Script executed:
#!/bin/bash # Check the minimum required Go version for testify/assert go list -m -versions github.com/stretchr/testify | grep -oP 'v[\d.]+'Length of output: 355
Action Required: Ensure Compatibility of Testify in Older Go Versions
The static analysis indicates that while the available versions of Testify (v1.1.1 through v1.10.0) cover a broad range, our CI and static analysis tools are failing to locate the
github.com/stretchr/testify/assertpackage when running on older Go versions (e.g., 1.7.6, 1.8.7, 1.9.7, 1.10.8). This discrepancy suggests that our dependency management might not be adequately constraining the Testify package for legacy Go environments.Consider one or a combination of the following improvements:
- Add a version constraint in your
go.mod: Pin Testify to a version that is verified to work with all supported Go versions.- Update the CI configuration: Skip running tests on Go versions known to have compatibility issues with Testify.
- Add a build constraint in
opts_test.go: Prevent this test file from compiling in unsupported Go versions (or use a separateconstraints.gofile to handle such dependencies).Please review these options and adjust the dependency constraints or CI/build configurations accordingly.
🧰 Tools
🪛 GitHub Check: Go 1.10.8 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.9.7 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.8.7 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:🪛 GitHub Check: Go 1.7.6 test
[failure] 4-4:
cannot find package "github.com/stretchr/testify/assert" in any of:platforms_output_test.go (1)
3-9: Excellent data source for platform compatibility testingThis comprehensive mapping of Go versions to their supported platforms will be invaluable for ensuring backward compatibility and precise platform support across different Go versions.
toolchain.go (3)
51-51: Consistent use of version parameterGood use of the version parameter for determining supported platforms.
96-96: Improved OS detection with constantsUsing the
goosWindowsconstant instead of a hard-coded string improves maintainability.
18-19:✅ Verification successful
Improved function signature with structured version type
Refactoring to use a structured version type (
*version.Version) instead of a string improves type safety and makes the code more maintainable.I notice that
gobinis used here but it's not defined in this file. Make sure it's properly defined elsewhere in the codebase.
🏁 Script executed:
#!/bin/bash # Check if gobin is defined somewhere in the codebase grep -r "gobin =" --include="*.go" . grep -r "var gobin" --include="*.go" . grep -r "const gobin" --include="*.go" .Length of output: 147
Action: Confirm definition and usage of
gobinacross the codebase
- The refactoring to use a structured version type (
*version.Version) is an excellent improvement for type safety.- The verification indicates that
gobinis defined inconst.go(e.g.,gobin = "go"), which addresses the concern about its declaration.- Please ensure that this definition is correctly imported and accessible where needed.
platform_test.go (3)
12-19: Well-implemented version parsing utilityThe
MustParseVersionfunction provides a convenient way to convert Go version strings toversion.Versionobjects, with appropriate error handling.One minor note: The function assumes that the input string starts with "go" and skips those characters when parsing. This works well for your use case but may not be obvious to other developers. A comment explaining this assumption would be helpful.
func MustParseVersion(in string) *version.Version { + // Assumes input is in format "goX.Y.Z" and strips the "go" prefix v, err := version.NewVersion(in[2:]) if err != nil { panic(err) } return v }
171-208: Comprehensive test for platform compatibilityThe new
TestPlatformsGoOutputfunction is an excellent addition that verifies the platforms returned bySupportedPlatformsmatch the expected output from thego tool dist listcommand for each Go version.
135-135: Consistent handling of nil for unknown versionsThe change to pass
nilfor unknown versions is good and aligns with the function signature change.go.go (7)
36-36: No functional change visible.
This line change appears to be whitespace or formatting only.
47-47: Use of constant for Windows OS is a good practice.
This refactor improves maintainability by referencinggoosWindowsrather than a string literal.
64-65: Consistent usage ofgoosWindows.
Ensures uniform OS checks across the codebase and prevents typos.
87-87: Centralized environment handling.
Switching toexecGo(opts.GoCmd, opts.Env(), chdir, args...)clarifies responsibilities for environment setup.
127-127: Consistent usage ofgobin.
Replacing "go" with thegobinconstant unifies command references and makes updates easier.
150-150: Redirecting stdout to an in-memory buffer.
Capturing output withstdoutallows post-processing of command results.
83-83:❓ Verification inconclusive
Verify completeness of build arguments.
Ensureopts.Arguments()incorporates all necessary compiler and linker flags for your use case.
🏁 Script executed:
#!/bin/bash # Searches for references to opts.Arguments() across the codebase rg 'opts\.Arguments\(' -A 3 -B 3Length of output: 223
Action Required: Validate Comprehensive Build Arguments
Please verify that the implementation of
opts.Arguments()truly returns all the necessary compiler and linker flags for your intended build scenarios. Our search confirms its sole usage in the build command construction ingo.go(line 83 and following), where the build args are assembled via:args := append([]string{"build"}, opts.Arguments()...) ... args = append(args, "-o", outputPathReal, opts.PackagePath)Ensure that:
- The output of
opts.Arguments()is complete and tailored to your use case.- There aren’t any required flags missing from its returned slice.
platform.go (22)
45-51: Clearer removal logic with a switch statement.
This approach is more explicit than conditional chaining and ensures each scenario is distinctly handled.
60-70: Revising Platforms_1_0 with named OS/arch constants.
UsinggoosDarwin,goarch386, etc. promotes clarity and avoids string literal mismatches.
73-86: Platforms_1_1 definition and 1.2 inheritance.
Introducing additional OS/arch combos and reusing the prior version preserves consistency.
87-98: Platforms_1_3 introduction.
Adding Dragonfly, NaCl, and Solaris expands coverage in line with Go 1.3’s official support.
99-105: Platforms_1_4 extension.
Adding Android (ARM) and Plan9 (AMD64) aligns with historical Go 1.4 changes.
107-116: Platforms_1_5 extension.
Includes Darwin ARM/ARM64 and more Linux architectures. Reflects Go 1.5’s broadening platform support.
118-129: Platforms_1_6 extension.
Incorporates Android 386/AMD64 and MIPS64 expansions aligned with Go 1.6.
130-148: Platforms_1_7 extension.
Refined mips64 support, s390x addition, and partial NaCl adjustments track Go 1.7’s updates.
150-170: Platforms_1_8 and subsequent versions through 1.11.
Progressive additions and removals match evolving Go support until 1.11.
172-177: Platforms_1_12 extension.
Adds AIX, Windows-ARM, removing or refining older platform references in line with Go 1.12.
182-190: Platforms_1_13 extension.
Introduces illumos, additional ARM64 coverage consistent with Go 1.13 progressions.
191-202: Platforms_1_14 extension.
Adds FreeBSD ARM64, Linux RISC-V 64, and drops some NaCl references consistent with Go 1.14.
204-210: Platforms_1_15 updates.
Deprecates Darwin 386/ARM, reflecting official support changes around Go 1.15.
212-221: Platforms_1_16 updates.
Adds iOS, further Darwin ARM64, and relevant Android expansions for Go 1.16.
223-229: Platforms_1_17 updates.
Incorporates Windows ARM64 as documented in Go 1.17.
230-235: Platforms_1_18 updates.
Reintroduces illumos AMD64 coverage as per Go 1.18 expansions.
237-242: Platforms_1_19 updates.
Adds Linux loong64 support aligned with Go 1.19 expansions.
244-249: Platforms_1_20 updates.
Highlights FreeBSD RISC-V 64 additions introduced in Go 1.20.
251-258: Platforms_1_21 updates.
Brings WASI coverage, dropping MIPS64 for OpenBSD. Reflects Go 1.21 changes.
262-263: Platforms_1_22 fallback.
No newly added platforms, continuing the earlier set for consistency.
267-275: *SupportedPlatforms refactor to accept version.Version.
Switching from string-based checks to typed version constraints enhances clarity and reduces parse errors.
281-286: PlatformConstraint struct definition.
Associating version constraints with a slice of platforms provides a scalable approach for version-based platform logic.constraints.go (2)
1-9: New constraints.go file scaffolding.
Global variables for flags, platforms, and experiments centralize version constraint logic.
11-99: Efficient initialization of version constraints.
Caching parsed constraints avoids redundant parsing and ensures consistent version checks across the codebase.main.go (13)
6-6: No issues with the introduced "log" import.Usage of "log" for output messages is appropriate.
47-50: No functional issues observed with these flag definitions.The code properly binds the platform flags to the custom
PlatformFlagtype.
52-52: Trivial comment change.No concerns here—this comment remains consistent with the surrounding context.
56-56: Ensure relative vs absolute paths are handled thoroughly.The
-change-dirflag can lead to confusion if the path doesn't exist or is relative.Would you like to add path validation or directory existence checks before proceeding with the build? If so, I can propose a helper function to verify this at runtime.
58-62: New flags for sanitizers and coverage look good.These additional flags should help with advanced debugging and coverage analysis.
105-111: Error handling for Go version retrieval is correctly implemented.The approach of printing an error message and returning an error code is clear and sufficient.
120-121: Passing the parsed version tomainBuildToolchainis a clean approach.Ensures full compatibility with the new version-aware logic.
131-131: Good consistency in passing the version tomainListOSArch.This keeps platform logic in sync with the new version parsing flow.
149-149: Fetching platforms viaSupportedPlatforms(v)is a neat integration.By hooking the version parameter here, you ensure OS/Arch resolution respects go-version constraints.
157-158: Informative console message about parallel builds.Printing the number of parallel builds helps users diagnose performance or concurrency issues.
161-163: Introducing concurrency locks is well-managed.Using a
sync.MutexandWaitGroupto handle parallel builds looks correct.
166-166: Semaphore channel usage for parallelism is appropriate.This typical pattern ensures no more than N builds proceed concurrently.
178-208: CompileOpts usage is consistent with recently added flags.All fields align well with their respective variables, enabling robust cross-compilation settings.
opts.go (3)
62-86:Env()method comprehensively sets environment variables.No issues spotted; the approach is straightforward and clear.
88-117: Gathering build arguments viaBuildArgFlagsis tidy and modular.Splitting logic into AddString, AddBoolean, etc., is a clean approach.
119-175: All BuildArgFlags methods integrate seamlessly.The
ShouldSkipVersion,AddStrings,AddString, andAddBooleanpatterns are easy to maintain and reason about.const.go (3)
3-5: Centralizing the Go binary name is convenient.Having a single definition for
gobinsimplifies referencing across files.
7-30: Flag name constants are logically organized.Centralizing these references prevents typos and eases future renaming.
52-69:❓ Verification inconclusive
GOARCH constants are broad.
You might periodically synchronize these with new releases of Go.
(Remove or update any stale architectures based on the diff as appropriate.)
🏁 Script executed:
#!/bin/bash # Compare listed architectures with those supported by the current Go toolchain go tool dist list | cut -d/ -f2 | sort -u > distro_arches.txt echo "User-defined arches (from const.go):" cat <<EOF > user_arches.txt 386 amd64 amd64p32 arm arm64 mips mipsle mips64 mips64le s390x ppc64 ppc64le riscv64 loong64 wasm EOF echo "Diff between user_arches.txt and distro_arches.txt:" diff distro_arches.txt user_arches.txtLength of output: 382
Sync GOARCH Constants with the Go Toolchain
The diff output reveals that the list in
const.godoes not perfectly match the architectures reported bygo tool dist list. In particular:
Extra entry:
amd64p32exists inconst.gobut isn’t present in the Go tool’s supported list.Ordering/value discrepancies:
- Differences for
loong64,mipsle, ands390xsuggest that their positions or presence might not be aligned with the Go tool's output.Please manually verify the list from
go tool dist liston your setup and updateconst.goaccordingly to ensure these constants remain in sync with the latest Go releases.
| {"ShouldReturnBooleanMiscMixed", &CompileOpts{GoVersion: MustParseVersion("go1.13"), ModCacheRW: true, TrimPath: true, Rebuild: true}, []string{"-a", "-trimpath"}}, | ||
| {"ShouldReturnStringC", &CompileOpts{GoVersion: MustParseVersion("go1.20"), ChangeDir: "foo"}, []string{"-C=foo"}}, | ||
| {"ShouldReturnStringCoverPackage", &CompileOpts{GoVersion: MustParseVersion("go1.20"), CoverPackage: "foo"}, []string{"-coverpkg=foo"}}, | ||
| {"ShouldReturnStringsASMFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), ASMFlags: "foo"}, []string{`-asmflags="foo"`}}, |
There was a problem hiding this comment.
Fix test expectations for command line flags with quoted values
The test is failing because the expected format for the command line flags is different from the actual implementation:
TestCompileOpts_Arguments/ShouldReturnStringsASMFlags failed: expected: []string{"-asmflags=\"foo\""}, actual: []string{"-asmflags", "foo"}
This pattern repeats for ASMFlags, GcCGOFlags, GcFlags, LDFlags, and Tags.
Update the test expectations to match the actual implementation:
- {"ShouldReturnStringsASMFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), ASMFlags: "foo"}, []string{`-asmflags="foo"`}},
+ {"ShouldReturnStringsASMFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), ASMFlags: "foo"}, []string{"-asmflags", "foo"}},
- {"ShouldReturnStringsGcCGOFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), GcCGOFlags: "foo"}, []string{`-gccgoflags="foo"`}},
+ {"ShouldReturnStringsGcCGOFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), GcCGOFlags: "foo"}, []string{"-gccgoflags", "foo"}},
- {"ShouldReturnStringsGcFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), GcFlags: "foo"}, []string{`-gcflags="foo"`}},
+ {"ShouldReturnStringsGcFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), GcFlags: "foo"}, []string{"-gcflags", "foo"}},
- {"ShouldReturnStringsLDFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), LDFlags: "foo"}, []string{`-ldflags="foo"`}},
+ {"ShouldReturnStringsLDFlags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), LDFlags: "foo"}, []string{"-ldflags", "foo"}},
- {"ShouldReturnStringsTags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), Tags: "foo"}, []string{`-tags="foo"`}},
+ {"ShouldReturnStringsTags", &CompileOpts{GoVersion: MustParseVersion("go1.20"), Tags: "foo"}, []string{"-tags", "foo"}},Also applies to: 34-34, 35-35, 37-37, 43-43
No description provided.