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.

Sign up for GitHub

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

Jump to bottom

bazel,colexec: generate crossjoiner.eg.go within bazel #59056

Merged
merged 1 commit into from
Jan 15, 2021
Merged

bazel,colexec: generate crossjoiner.eg.go within bazel #59056

merged 1 commit into from
Jan 15, 2021

Conversation

alan-mas
Copy link
Contributor

@alan-mas alan-mas commented Jan 15, 2021 •
edited by irfansharif
Loading

We broke the bazel build in 52c5f51 when we introduced a new .eg.go
file, without updating the bazel target. We do that here.

Fixes #59052

@blathers-crl
Copy link

blathers-crl bot commented Jan 15, 2021

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Jan 15, 2021
@blathers-crl blathers-crl bot requested a review from rickystewart January 15, 2021 18:03
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@alan-mas alan-mas requested a review from irfansharif January 15, 2021 18:03
@alan-mas
Copy link
Contributor Author

@irfansharif would you mind to confirm the variables part on the commits? (That what I am not totally sure if I am missing something) I think I am missing type but I am not sure how to include it

// Workaround for bazel auto-generated code. goimports does not automatically
// pick up the right packages when run within the bazel sandbox.
var (
	_ = typeconv.DatumVecCanonicalTypeFamily

)

@rickystewart
Copy link
Collaborator

@irfansharif would you mind to confirm the variables part on the commits? (That what I am not totally sure if I am missing something) I think I am missing type but I am not sure how to include it

// Workaround for bazel auto-generated code. goimports does not automatically
// pick up the right packages when run within the bazel sandbox.
var (
	_ = typeconv.DatumVecCanonicalTypeFamily

)

_ = types.Unknown?

@blathers-crl
Copy link

blathers-crl bot commented Jan 15, 2021

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

@alan-mas
Copy link
Contributor Author

_ = types.Unknown

Just tried and bazel build //pkg/cmd/cockroach-short --sandbox_debug is working as well as make build let see what TC says about it

@irfansharif
Copy link
Contributor

Thanks for the patch!

bors r+

@irfansharif irfansharif changed the title build: Fixed build broken at head bazel,colexec: generate crossjoiner.eg.go within bazel Jan 15, 2021
@alan-mas
Copy link
Contributor Author

Thanks for the patch!

bors r+

And about the vars, is all good? @irfansharif

// Workaround for bazel auto-generated code. goimports does not automatically
// pick up the right packages when run within the bazel sandbox.
var (
	_ = typeconv.DatumVecCanonicalTypeFamily

)

I added what Ricky suggested, but seems that you took them away, right? (so might be wrong that change from my side hahah)

@irfansharif
Copy link
Contributor

Yup!

@craig
Copy link
Contributor

craig bot commented Jan 15, 2021

Build failed (retrying...):

rickystewart
rickystewart reviewed Jan 15, 2021
View reviewed changes
pkg/sql/colexec/crossjoiner_tmpl.go Outdated
// pick up the right packages when run within the bazel sandbox.
var (
_ = typeconv.DatumVecCanonicalTypeFamily

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the linter has an issue with this blank line?

@craig
Copy link
Contributor

craig bot commented Jan 15, 2021

Canceled.

@rickystewart
Copy link
Collaborator

bors r+

@alan-mas
bazel,colexec: generate crossjoiner.eg.go within bazel
651a537 
We broke the bazel build in 52c5f51 when we introduced a new .eg.go
file, without updating the bazel target. We do that here.

Fixes: cockroachdb#59052.

Release note: None
@craig
Copy link
Contributor

craig bot commented Jan 15, 2021

Canceled.

@rickystewart
Copy link
Collaborator

bors r+

@craig
Copy link
Contributor

craig bot commented Jan 15, 2021

Build succeeded:

@craig craig bot merged commit ff1442b into cockroachdb:master Jan 15, 2021
@alan-mas alan-mas deleted the 59052-broken branch January 15, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rickystewart rickystewart rickystewart approved these changes

@irfansharif irfansharif Awaiting requested review from irfansharif

Assignees
No one assigned
Labels
O-community Originated from the community X-blathers-triaged blathers was able to find an owner
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build/bazel: build is broken at HEAD
4 participants
@alan-mas @cockroach-teamcity @rickystewart @irfansharif