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

dev: various improvements to dev generate cgo and friends #82418

Merged
merged 1 commit into from
Jun 7, 2022

Conversation

rickystewart
Copy link
Collaborator

  1. Up until this point, dev generate go has not included the
    zcgo_flags.go sources, which has been a point of confusion for
    people who expect dev generate go to generate all the .go files.
    Now dev generate go includes cgo as well, and there is a new
    target dev generate go_nocgo that does what dev generate go used
    to do.
  2. Now dev generate cgo is conscious of where force_build_cdeps is
    set. If it is, then we make sure not to check in one of the
    pre-archived locations. To this end we add a test_force_build_cdeps
    target that dev generate cgo builds.

Release note: None

@rickystewart rickystewart requested a review from a team as a code owner June 3, 2022 17:47
@cockroach-teamcity
Copy link
Member

This change is Reviewable

1. Up until this point, `dev generate go` has not included the
   `zcgo_flags.go` sources, which has been a point of confusion for
   people who expect `dev generate go` to generate all the .go files.
   Now `dev generate go` includes `cgo` as well, and there is a new
   target `dev generate go_nocgo` that does what `dev generate go` used
   to do.
2. Now `dev generate cgo` is conscious of where `force_build_cdeps` is
   set. If it is, then we make sure not to check in one of the
   pre-archived locations. To this end we add a `test_force_build_cdeps`
   target that `dev generate cgo` builds.

Release note: None
Copy link
Contributor

@mari-crl mari-crl left a comment

Choose a reason for hiding this comment

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

LGTM!! :D

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @mari-crl and @rickystewart)


pkg/cmd/dev/generate.go line 86 at r1 (raw file):

		targetsMap[target] = struct{}{}
	}
	{

What's the goal of this block here and the one below? Just for style, to make it clearer that these belong together?

Copy link
Contributor

@mari-crl mari-crl left a comment

Choose a reason for hiding this comment

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

:lgtm:

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

@rickystewart
Copy link
Collaborator Author

TFTR!

What's the goal of this block here and the one below? Just for style, to make it clearer that these belong together?

The blocks just open up a new scope. In this case I have some similarly named variables between the two blocks and I don't want to accidentally reference one when I mean the other.

bors r=mari-crl

@craig
Copy link
Contributor

craig bot commented Jun 7, 2022

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Jun 7, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Jun 7, 2022

Build succeeded:

@craig craig bot merged commit 0c8f826 into cockroachdb:master Jun 7, 2022
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.

None yet

3 participants