Skip to content
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

bzltestutil: move os.Chdir call into new package #3681

Merged
merged 3 commits into from
Sep 7, 2023

Conversation

jayconrod
Copy link
Contributor

What type of PR is this?

Bug fix

What does this PR do? Why is it needed?

Sorry for missing this the first time in #3679. The package init algorithm was a little different than I thought, and I imagined that Go 1.21 was already in use, so I didn't look into it deeply.

The problem was that even though "+initfirst/.../bzlutil" sorts lower than other packages, it imports "sync" and a number of other std packages that sort higher than "github.com/..." user packages. If a user package has no dependencies, it's eligible to be initialized before bzltestutil, even with the lower name.

I moved the init function with the os.Chdir call into a separate tiny package that only imports "os" and "runtime". It should get initialized much earlier.

Which issues(s) does this PR fix?

Fixes #3675

Sorry for missing this the first time in bazelbuild#3679. The package init algorithm
was a little different than I thought, and I imagined that Go 1.21
was already in use, so I didn't look into it deeply.

The problem was that even though "+initfirst/.../bzlutil" sorts lower than
other packages, it imports "sync" and a number of other std packages
that sort higher than "github.com/..." user packages. If a user package
has no dependencies, it's eligible to be initialized before bzltestutil,
even with the lower name.

I moved the init function with the os.Chdir call into a separate tiny package
that only imports "os" and "runtime". It should get initialized much earlier.

Fixes bazelbuild#3675
@jayconrod
Copy link
Contributor Author

@fmeum I think this will fix the issue in #3679. I've verified at least that my company's affected vendored tests now pass when using 1.21.

go/tools/bzltestutil/BUILD.bazel Outdated Show resolved Hide resolved
go/tools/bzltestutil/chdir/init.go Show resolved Hide resolved
go/tools/bzltestutil/chdir/init.go Outdated Show resolved Hide resolved
@sluongng
Copy link
Contributor

sluongng commented Sep 7, 2023

If you want to future proof this, you could add github.com/klauspost/compress into https://github.com/bazelbuild/rules_go/blob/master/tests/integration/popular_repos/popular_repos.py

I think that repo is quite relevant for the Bazel and Go ecosystem in general (we use it as well).

@jayconrod
Copy link
Contributor Author

If you want to future proof this, you could add github.com/klauspost/compress into https://github.com/bazelbuild/rules_go/blob/master/tests/integration/popular_repos/popular_repos.py

I think that repo is quite relevant for the Bazel and Go ecosystem in general (we use it as well).

I tried, but it's not as easy as I hoped. Most of the tests in that repo read testdata files from a top-level testdata directory, so some hand-edited filegroups and data attributes would be needed: Gazelle / go_repository can't do it automatically. We've excluded tests in org_golang_x_tools for similar reasons.

Some tests are slow: zstd takes 99s on my machine.

There's also some desync between popular_repos.py and rules_go's dependencies.

@fmeum fmeum merged commit f03a723 into bazelbuild:master Sep 7, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

go_test: I/O in package init function fails after Go 1.21 due to initialization order change
3 participants