-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
dev,generate-cgo: externalize cgo files gen into a seperate binary #104554
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
pkg/cmd/generate-cgo/main.go
Outdated
} | ||
|
||
func getExecutionRoot() (string, error) { | ||
args := []string{"info", "execution_root", "--color=no"} |
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.
Doesn't really make much sense that workspace
and bazel-bin
have to be passed in, but execution_root
does not. I would say just have generate-cgo
look all of them up by itself.
"pkg/geo/geoproj", | ||
args := []string{ | ||
"run", | ||
"//pkg/cmd/generate-cgo:generate-cgo", |
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.
Should add:
"--run_under=cd [workspace] && ",
Or something like this to make sure the binary is executed in the workspace directory.
Previously, we only needed to gen cgo files using `dev` so that was were that logic lived. Now, we need to gen cgo files outside dev to fix an issue with gcassert [see cockroachdb#65485]. This code change externalizes the code into a separate binary and let's `dev gen cgo` point to it. Release note: None Epic: None Informs cockroachdb#65485
38e5f3f
to
83d4e72
Compare
bors r=rickystewart |
Build failed: |
bors retry |
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: {"message":"Changes must be made through a pull request.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
bors retry |
bors r- |
Canceled. |
bors r+ single-on |
bors r- |
bors single on |
bors r+ single on |
Build failed: |
bors r+ single on |
Build succeeded: |
Previously, we only needed to gen cgo files using
dev
so that was were that logic lived. Now, we need to gen cgo files outside dev to fix an issue with gcassert [see #65485].This code change externalizes the code into a separate binary and let's
dev gen cgo
point to it.Release note: None
Epic: None
Informs #65485