Skip to content

Conversation

@knz
Copy link
Contributor

@knzknz commented Apr 13, 2021

This change is Reviewable

@knzknz requested a review from erikgrinakerApril 13, 2021 12:51
Copy link
Contributor

@erikgrinakererikgrinaker left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @knz)


Makefile.update-protos, line 7 at r1 (raw file):

# go get github.com/cockroachdb/protoc-gen-gogoroach # go get github.com/gogo/protobuf/types # go get github.com/gogo/protobuf/protoc-gen-gogo 

Could we somehow lock these versions in go.mod, even though they're build tools rather than direct dependencies?

@knz
Copy link
ContributorAuthor

knz commented Apr 13, 2021

Could we somehow lock these versions in go.mod, even though they're build tools rather than direct dependencies?

Maybe; how should I do this?

@erikgrinaker
Copy link
Contributor

Maybe; how should I do this?

You could do something like this: https://marcofranssen.nl/manage-go-tools-via-go-modules/

The Go team approves: golang/go#25922 (comment)

@erikgrinaker
Copy link
Contributor

Also, I seem to recall that @tooolbox had some ideas on handling build-time dependencies.

@knzknz mentioned this pull request Apr 13, 2021
@knz
Copy link
ContributorAuthor

knz commented Apr 13, 2021

Rebased; also:

Could we somehow lock these versions in go.mod, even though they're build tools rather than direct dependencies?

In fact, not yet. I had to revert that doc change in the Makefile - for now, we still prefer to use crdb's own protoc-gen-gogo to ensure that the generated structs are simplified.

So that point is not yet relevant.

@erikgrinaker
Copy link
Contributor

Cool, let's revisit later.

@knzknz merged commit 72ec752 into cockroachdb:masterApr 13, 2021
@knzknz deleted the 20210413-proto branch April 13, 2021 16:52
Sign up for freeto 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.

2 participants

@knz@erikgrinaker