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

bazel: stage all go source files in a temp dir named after the package #65094

Merged
merged 1 commit into from
May 13, 2021

Conversation

rickystewart
Copy link
Collaborator

This works around an issue where Go source file names as stored in the
cockroach binary were truncated (e.g., with the leading
github.com/cockroachdb/cockroach prefix, or even the entire package
name prefix, removed). This breaks unit tests (#61913) and some other
internal stuff.

We solve this by staging all Go source files during the build in a
temporary directory named after the package. This incurs an additional
I/O cost, but for now while our codebase isn't able to deal with the
differing file names, we can deal with it.

Fixes #64379
See also #64383

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rickystewart rickystewart requested a review from rail May 12, 2021 22:13
@rickystewart
Copy link
Collaborator Author

cc @knz

@rickystewart
Copy link
Collaborator Author

@knz The tests from #61913 are still failing, but I think that has to do with the fact that util/caller isn't prepared to deal with the case where filenames aren't absolute + prefixed by the gopath. I'll leave that as a follow-up, not really sure how you think that should be fixed.

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Ooooh shiny ✨
I went to look at your change in rules_go: cockroachdb/rules_go@a94a425

this looks reasonable to me.

I think this change is generally beneficial even though it doesn't fix the failing test right away. Thank you a lot for looking into it.

Reviewed 1 of 1 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @rail)

Copy link
Member

@rail rail left a comment

Choose a reason for hiding this comment

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

:lgtm:

Woot!

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rickystewart)

@rickystewart
Copy link
Collaborator Author

One broken unit test, let me take care of that really quick...

This works around an issue where Go source file names as stored in the
`cockroach` binary were truncated (e.g., with the leading
`github.com/cockroachdb/cockroach` prefix, or even the entire package
name prefix, removed). This breaks unit tests (cockroachdb#61913) and some other
internal stuff.

We solve this by staging all Go source files during the build in a
temporary directory named after the package. This incurs an additional
I/O cost, but for now while our codebase isn't able to deal with the
differing file names, we can deal with it.

Fixes cockroachdb#64379
See also cockroachdb#64383

Release note: None
@rickystewart
Copy link
Collaborator Author

pkg/util/json/encode_test.go was overly complicated, works fine once you replace it with Bazel-friendly utilities.

bors r=rail,knz

@craig
Copy link
Contributor

craig bot commented May 13, 2021

Build succeeded:

@craig craig bot merged commit a40c777 into cockroachdb:master May 13, 2021
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.

build: bazel-generated go binaries should include a synthetic prefix
4 participants