Skip to content

Check in generated protobufs for Go - #36523

Closed
jhseu wants to merge 1 commit into
tensorflow:masterfrom
jhseu:vendored_protobufs
Closed

Check in generated protobufs for Go#36523
jhseu wants to merge 1 commit into
tensorflow:masterfrom
jhseu:vendored_protobufs

Conversation

@jhseu

@jhseu jhseu commented Feb 6, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

To make it more idiomatic and easier to install. See #35133

Gunhan, any objections to doing this? It adds 1.3 MB of generated files to the repo.

@jhseu
jhseu requested review from frankchn and gunan February 6, 2020 22:29
@tensorflow-bot tensorflow-bot Bot added the size:XL CL Change Size:Extra Large label Feb 6, 2020
@tensorflow-bot tensorflow-bot Bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Feb 6, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Feb 6, 2020
@gbaned gbaned self-assigned this Feb 7, 2020
@gbaned gbaned added prtype:bugfix PR to fix a bug ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Feb 7, 2020
This was referenced Feb 7, 2020
Closed
Closed
@mathisve mathisve mentioned this pull request Mar 3, 2020
Closed
@jhseu jhseu mentioned this pull request Mar 10, 2020
Merged
@gunan

gunan commented Apr 22, 2020

Copy link
Copy Markdown
Contributor

Did we not do this internally?
I am confused?

@gbaned

gbaned commented May 12, 2020

Copy link
Copy Markdown
Contributor

@jhseu, @frankchn Any update on this PR? Please. Thanks!

@JacobSMoller

JacobSMoller commented May 13, 2020

Copy link
Copy Markdown

Any update on when this will be merged?

@ntklink

ntklink commented May 19, 2020

Copy link
Copy Markdown

Why Merging is blocked?

@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jun 2, 2020
@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jun 17, 2020
@ndbls ndbls mentioned this pull request Jun 24, 2020
Closed
@philpearl

philpearl commented Jun 24, 2020

Copy link
Copy Markdown
Contributor

please please please merge this PR. Every other Go package we use we can just "go get". I really don't want to have to have special case code in our build for tensorflow.

@gbaned

gbaned commented Jul 14, 2020

Copy link
Copy Markdown
Contributor

@jhseu, @frankchn Any update on this PR? Please. Thanks!

@frankchn

frankchn commented Jul 15, 2020

Copy link
Copy Markdown
Contributor

There is some issue merging this in where the CL doesn't seem to be able to be merged properly internally. @jhseu might know more

@philpearl

philpearl commented Jul 16, 2020

Copy link
Copy Markdown
Contributor

How to build tensorflow

  1. Get a job at Google
  2. ...
  3. ...
  4. 💰

@gbaned gbaned added ready to pull PR ready for merge process and removed ready to pull PR ready for merge process labels Jul 16, 2020
@jhseu

jhseu commented Aug 26, 2020

Copy link
Copy Markdown
Contributor Author

We had a discussion with the Go team who discouraged checking this in because other projects had faced issues with conflicts between generated protobufs and the protobuf files generated by bazel. Closing for now.

Users should run go generate as mentioned in the README.

@jhseu jhseu closed this Aug 26, 2020
@philpearl

philpearl commented Aug 27, 2020

Copy link
Copy Markdown
Contributor

please please please merge this PR. Every other Go package we use we can just "go get". I really don't want to have to have special case code in our build for tensorflow.

No other package I've ever seen does this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gunan gunan Awaiting requested review from gunan
1 more reviewer
@frankchn frankchn frankchn approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

@gbaned gbaned

Labels

cla: yes prtype:bugfix PR to fix a bug ready to pull PR ready for merge process size:XL CL Change Size:Extra Large

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@jhseu @gunan @gbaned @JacobSMoller @ntklink @philpearl @frankchn @googlebot @kokoro-team