-
Notifications
You must be signed in to change notification settings - Fork 919
GODRIVER-3573: Remove old duplicate compilation test #2257
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
🧪 Performance ResultsCommit SHA: fefbb99The following benchmark tests for version 694975c744c6260007649c54 had statistically significant changes (i.e., |z-score| > 1.96):
For a comprehensive view of all microbenchmark results for this PR's commit, please check out the Evergreen perf task for this patch. |
API Change ReportNo changes found! |
6264939 to
88a9fbf
Compare
cleanup gitignore remove cmd/compilecheck from Taskfile
and allow compile test on specific versions resolve deprecated BindMount function call remove unwanted files
add docker dependency as direct dep for compilecheck
Go 1.19 is the current minimum supporter Go version set go toolchain to auto to allow proper version to be downloaded for build
This is to allow the test to continue to work until GODRIVER-3723
88a9fbf to
53192c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR removes duplicate compilation tests by eliminating the old internal/cmd/compilecheck test in favor of the newer containerized test in internal/test/compilecheck. The changes improve test flexibility by allowing developers to specify Go versions via environment variables and update the container binding mechanism to use the non-deprecated HostConfigModifier API.
- Removed old compilation test directory and associated files
- Enhanced containerized test to support configurable Go versions via
GO_VERSIONSenvironment variable - Updated container configuration to use
HostConfigModifierinstead of deprecatedBindMountfunction
Reviewed changes
Copilot reviewed 10 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/test/compilecheck/compile_check_test.go |
Refactored to inline test code, use HostConfigModifier for volume binding, and support configurable Go versions through environment variable |
internal/test/compilecheck/go.mod |
Promoted github.com/docker/docker from indirect to direct dependency |
internal/cmd/compilecheck/main.go |
Deleted old test main file |
internal/cmd/compilecheck/go.mod |
Deleted old test module file |
internal/cmd/compilecheck/go.sum |
Deleted old test dependencies file |
internal/cmd/compilecheck/go.work |
Deleted old workspace file |
etc/compile_check.sh |
Deleted old bash script for compilation checks |
etc/run-compile-check-test.sh |
Updated to support configurable Go versions with default fallback |
go.work |
Removed reference to deleted internal/cmd/compilecheck directory |
Taskfile.yml |
Replaced build-compile-check task with compilecheck-119 using containerized test |
.github/workflows/codeql.yml |
Updated to run containerized compile check inline instead of using Taskfile |
.gitignore |
Removed entries for deleted compilation check binaries |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Taskfile.yml
Outdated
| GOWORK: off | ||
| GOTOOLCHAIN: auto | ||
| GO_VERSIONS: "1.19" | ||
| COMPILECHECK_USE_DOCKER: "0" |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The environment variable COMPILECHECK_USE_DOCKER is set but never used in the test code. This appears to be dead code that should be removed to avoid confusion.
| COMPILECHECK_USE_DOCKER: "0" |
.github/workflows/codeql.yml
Outdated
| go build ${BUILD_TAGS} ./... | ||
| go test -short ${BUILD_TAGS} -run ^$$ ./... | ||
| GO_VERSIONS="1.19" go test -v ./internal/test/compilecheck -run '^TestCompileCheck/golang:1.19$' |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compile check test should be run with GOWORK=off to match the configuration in the Taskfile (line 26) and the bash script at etc/run-compile-check-test.sh (line 18). Without this, the test may use workspace settings which could affect the compilation test behavior.
| GO_VERSIONS="1.19" go test -v ./internal/test/compilecheck -run '^TestCompileCheck/golang:1.19$' | |
| GOWORK=off GO_VERSIONS="1.19" go test -v ./internal/test/compilecheck -run '^TestCompileCheck/golang:1.19$' |
Remove COMPILECHECK_USE_DOCKER as it is unused Add GO_WORK=off to compilecheck test
| GOTOOLCHAIN: local | ||
| run: | | ||
| # TODO(GODRIVER-3723): Run using taskfile targets. | ||
| go build ./... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] We need to include the libmongocrypt task before building:
task install-libmongocrypt
If this doesn't work, try running the script manually:
bash etc/install-libmongocrypt.sh
test -d install || test -d /cygdrive/c/libmongocrypt/bin
If we have to do the latter, we should remove taskfile support:
- name: Install Taskfile support
uses: arduino/setup-task@v2
etc/run-compile-check-test.sh
Outdated
| if [ -z "${GO_VERSIONS:-}" ]; then | ||
| GO_VERSIONS="1.19,1.20,1.21,1.22,1.23,1.24,1.25" | ||
| fi | ||
| export GO_VERSIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] We should move the go versions to the compile check test. If we need to run a specific version(s), we can do that with the test name:
GO_WORK=off GO_VERSIONS="1.19" go test -v ./internal/test/compilecheck -run '^TestCompileCheck/golang:1.19$'GO_VERSIONS="1.19" is a redundancy, 1.19 is already specified in the name regex.
.github/workflows/codeql.yml
Outdated
| go build ${BUILD_TAGS} ./... | ||
| go test -short ${BUILD_TAGS} -run ^$$ ./... | ||
| GO_WORK=off GO_VERSIONS="1.19" go test -v ./internal/test/compilecheck -run '^TestCompileCheck/golang:1.19$' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] The environment variable is defined in the specifications as GOWORK. Is GO_WORK=off working?
Taskfile.yml
Outdated
| env: | ||
| GOWORK: off | ||
| GOTOOLCHAIN: auto | ||
| GO_VERSIONS: "1.19" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Can we lose all these env variables?
| Mounts: []testcontainers.ContainerMount{ | ||
| testcontainers.BindMount(rootDir, "/workspace"), | ||
| Image: image, | ||
| Cmd: []string{"tail", "-f", "/dev/null"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Can we add a note about what this does? Something like "keep container running to Exec commands into it"?
| } | ||
|
|
||
| // Standard build. | ||
| exitCode, outputReader, err := container.Exec(context.Background(), []string{"go", "build", "./..."}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] We also need to include dynamic linking, building with tags, and building with various architectures.
mongo-go-driver/etc/compile_check.sh
Lines 33 to 42 in ad40e61
| # Dynamic linking | |
| $BUILD_CMD -buildmode=plugin | |
| # Check build with tags. | |
| [[ -n "$BUILD_TAGS" ]] && $BUILD_CMD $BUILD_TAGS ./... | |
| # Check build with various architectures. | |
| for ARCH in "${ARCHITECTURES[@]}"; do | |
| GOOS=linux GOARCH=$ARCH $BUILD_CMD ./... | |
| done |
Build tags can be env-dependent. The architectures should be subtests:
archTests := []string{"amd64", "arm64", /* etc */}
for _, arch := range archTests {
t.Run(fmt.Sprintf("GOARCH=%s", arch), func(t *testing.T) {
// TODO
})
}Add more supported linux architectures Add dynamic linking compilation check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 11 out of 13 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // goVersions is the list of Go versions to test compilation against. | ||
| // To run tests for specific version(s), use the -run flag: | ||
| // | ||
| // go test -v -run '^TestCompileCheck/go:1.19$' | ||
| // go test -v -run '^TestCompileCheck/go:1\.(19|20)$' |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documentation comment states "To run tests for specific version(s), use the -run flag:" but doesn't explain why someone would want to do this or mention the common use case of running just the minimum version (1.19) for quick local validation. Consider adding context about when to use specific versions vs all versions (e.g., "For quick local validation, test against the minimum supported version. For comprehensive testing before releases, test all versions.").
|
@prestonvasquez I'm noticing on evergreen the 1.19 compile check test is the only one running in the |
| } | ||
|
|
||
| // goBuild constructs a build command that tries GOTOOLCHAIN=goX.Y.0 first, then falls back to goX.Y. | ||
| func goBuild(ver, workdir string, env, extraFlags []string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] Suggest generalizing this to execGo so that we can run any go command.
type goExecConfig struct {
version string // Optional: Go version to use with GOTOOLCHAIN. If empty, uses default.
env map[string]string // Optional: Additional environment variables.
}
func execGo(t *testing.T, c testcontainers.Container, cfg *goExecConfig, args ...string) string {
// execute go specific ommands in container.
}Here's a POC for execContainer:
func execContainer(t *testing.T, c testcontainers.Container, cmd string) string {
t.Helper()
exit, out, err := c.Exec(context.Background(), []string{"bash", "-lc", cmd})
require.NoError(t, err)
b, err := io.ReadAll(out)
require.NoError(t, err)
// If the command failed, show full output.
require.Equal(t, 0, exit, "command failed: %s", b)
s := string(b)
// Strip any leading non‑printable bytes (some Docker/TTY combos do this).
for len(s) > 0 && s[0] < 0x20 {
s = s[1:]
}
return s
}| // | ||
| // go test -v -run '^TestCompileCheck/go:1.19$' | ||
| // go test -v -run '^TestCompileCheck/go:1\.(19|20)$' | ||
| var goVersions = []string{"1.19", "1.20", "1.21", "1.22", "1.23", "1.24", "1.25"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[optional] Suggest doing the versions 1 per line so we can add a comment for 1.19 and 1.25:
var versions = []string{
"1.19", // Minimum supported Go version for mongo-driver v2
"1.20",
"1.21",
"1.22",
"1.23",
"1.24",
"1.25", // Test suite Go Version
}| require.NoError(t, container.Terminate(context.Background())) | ||
| }) | ||
|
|
||
| // Write main.go into the container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] Use the io.Reader from ContainerFile to write main.go:
func TestCompileCheck(t *testing.T) {
// [...]
req := testcontainers.ContainerRequest{
FromDockerfile: testcontainers.FromDockerfile{
Context: filepath.Join(cwd, "docker"),
Dockerfile: "build.Dockerfile",
PrintBuildLog: true,
},
Files: []testcontainers.ContainerFile{
{
Reader: strings.NewReader(mainSrc),
ContainerFilePath: "/workspace/main.go",
FileMode: 0o644,
},
},
Entrypoint: []string{"tail", "-f", "/dev/null"},
WorkingDir: "/workspace",
}| require.NoError(t, err) | ||
| require.Equal(t, 0, exitCode, "failed to write main.go: %s", output) | ||
|
|
||
| // Write go.mod into the container. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] Use the go toolchain to write the mod file:
// Initialize Go module and download dependencies for this version.
_ = execGo(t, container, &goExecConfig{version: "1.25"}, "mod", "init", "example.com/mymodule")
_ = execGo(t, container, &goExecConfig{version: "1.25"}, "mod", "tidy")
// Edit go.mod to set minimum version to what the driver claims
execContainer(t, container, fmt.Sprintf(`sed -i 's/^go 1.25.0$/%s/' /workspace/go.mod`, versions[0]))| }, | ||
| } | ||
| // Each version gets its own workspace to avoid conflicts when running in parallel. | ||
| workspace := fmt.Sprintf("/workspace-%s", ver) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] This isn't necessary, we can re-compile with different go versions within the same workspace.
| require.Equal(t, 0, exitCode, "build with build tags failed: %s", output) | ||
|
|
||
| // Build for each architecture. | ||
| for _, architecture := range architectures { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Can we do all of the logic within the arch for loop?
- Standard Build
- Dynamic Build
- Build with tags
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I don't see a reason it can't be done, I can update the code to do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bringing offline conversation on here for record.
So turns out the dynamic linking go build doesn't work across all go architecture compilation. It only supports the more major ones like x86 and arm. Further it doesn't seem to support cross compile well for example running x86 linux build on the arm mac doesn't work from what it looks like. So for now I will move that back to outside of the architectures subtest. Alternatively I could probably make a check to run in some architectures that are supported and skip others. But I don't know the context behind what the dynamic linking/does so wanted to ask your thoughts on that before doing anything. Adding on that, looks like the gssapi build tag also pulls in a C library. The root cause of these issues seems to be that CGO is disabled by default on cross compilation because it's assumed extra libraries aren't added to handle cross C compilation.
I have opted to keep the structure the same as before where we run a normal build, dynamic build, and build with tags in the host OS. Then a normal cross compile build across all supported linux distros. I also pinned the docker image to amd64 as that is the main case that will occur in CI and allows the same expected output when run locally on an arm developer laptop and CI.
| require.Equal(t, 0, exitCode, "build with build tags failed: %s", output) | ||
|
|
||
| // Build for each architecture. | ||
| for _, architecture := range architectures { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[blocking] Each iteration of architecture should be a new subtest.
@RafaelCenzano Both compile checks look correct to me:
|
Create a container from Dockerfile that contains required dependencies Use the created container across multiple go version compilation checks Co-authored-by: Preston Vasquez <[email protected]>
Use standard docker image to allow for local development instead of using a MongoDB container that isn't accessible outside of MonogDB platforms Co-authored-by: Preston Vasquez <[email protected]>
Missing go.mod: Added go.mod file to container workspace since Go 1.19+ requires module context to build. Incomplete go.sum: Changed go mod download to go mod tidy to populate all dependency checksums. ./... pattern broken: Changed go build ./... to go build main.go due to GOTOOLCHAIN incompatibility with wildcard patterns. Toolchain version format: Added || fallback to try both go1.21.0 and go1.21 formats for cross-version compatibility. Python packaging missing: Added python3-packaging to Dockerfile for libmongocrypt CMake on Python 3.13+. GSSAPI headers missing: Added libkrb5-dev to Dockerfile for -tags=gssapi compilation. Wrong libmongocrypt path: Fixed PKG_CONFIG_PATH from /root/install/lib/pkgconfig to /root/install/libmongocrypt/lib/pkgconfig.
Ensure the driver is copied and go mod is used replace the driver with the local version Create different directories to allow for compilation of each version in the same container Remove uneeded comments minor cleanup
…mongodb#2268) Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
This is the case that will run most often in CI, this also sets up if we prebuild the image
Create an execContainer and execGo to check output was expected and abstract container test calls Write to the image using testcontainers methods instead of cat Created go.mod through go commands Remove uneeded separated workspaces for go versions Each architecture is now a subtest "arch:amd64" for example
4693c9f to
44acf46
Compare
Sudo is unused and to follow guidelines and best practices as highlighted by Semgroup SAST anaylsis
GODRIVER-3573
Summary
Background & Motivation
A new compilation test was created using container in #1992 for GODRIVER-3493. There was duplicate tests for testing compilation across supported go versions. This PR removes the old test and updates the new one to allow for more flexibility for testing if a developer needs it.