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

Fix downloading protobuf dependency #23400

Merged
merged 5 commits into from Dec 6, 2018

Conversation

dcirne
Copy link
Contributor

@dcirne dcirne commented Oct 31, 2018

Protobuf has been failing due to its repository having dependencies. Downloading the .tar.gz form GitHub doesn't work because it does not bring its dependencies with it.

This PR introduces a function to clone a repository, initializa and update its submodules. The function is used to download protobuf as a repository dependency, rather than just a file.

The function is also implemented in a generic way, so if in the future other dependencies fall into the same situation, it can be reused.

With these changes, tensorflow/workspace.bzl goes back to using the commit sha in PROTOBUF_URLS and PROTOBUF_STRIP_PREFIX, rather than the tag of a release.

A command at the end of the download_dependencies.sh script was commented out. The comment says for it to be removed once protobug is fixed. Perhaps no longer necessary.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

@googlebot
Copy link

CLAs look good, thanks!

@dcirne dcirne changed the title Fix downloading protobof dependency Fix downloading protobuf dependency Oct 31, 2018
@Harshini-Gadige Harshini-Gadige self-assigned this Oct 31, 2018
@Harshini-Gadige Harshini-Gadige added the awaiting review Pull request awaiting review label Oct 31, 2018
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Nov 2, 2018
@Harshini-Gadige Harshini-Gadige added the awaiting review Pull request awaiting review label Nov 8, 2018
@dcirne
Copy link
Contributor Author

dcirne commented Nov 15, 2018

Conflict has been resolved. The PR is ready for review.

@Harshini-Gadige
Copy link

Harshini-Gadige commented Nov 19, 2018

@therc Can I proceed with next steps to merge this PR ? I don't see "approved" check and so want to get clarified on proceeding. Please keep me posted.

@therc
Copy link

therc commented Nov 19, 2018

I'm not a tensorflow project member, I only added a drive-by comment. :)

therc
therc previously approved these changes Nov 19, 2018
@tensorflowbutler tensorflowbutler removed the awaiting review Pull request awaiting review label Nov 20, 2018
@dcirne
Copy link
Contributor Author

dcirne commented Nov 21, 2018

Hi @Harshini-Gadige , it would be great to see this PR merged. I have been using this for a while, without any issues. Is there anything I can do to help?

@Harshini-Gadige
Copy link

@dcirne Waiting for the reviewer approval.
@petewarden @wolffg - Any update ?

@dcirne
Copy link
Contributor Author

dcirne commented Nov 29, 2018

Resolved a conflict that appeared since our last contact.

@dcirne
Copy link
Contributor Author

dcirne commented Dec 4, 2018

@Harshini-Gadige with the news that 1.13 will be branching on Dec 13th, is there any chance this PR gets merged before that, in time to make the cut?

@Harshini-Gadige
Copy link

@dcirne Waiting for the reviewer approval.
@petewarden @wolffg - Any update ?

@petewarden @wolffg - Please review and approve this PR. If you are not the right person, please assign to the respective person.

@Harshini-Gadige Harshini-Gadige removed the request for review from wolffg December 4, 2018 20:24
@penpornk penpornk added the kokoro:force-run Tests on submitted change label Dec 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 6, 2018
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

I'm so sorry for our delay! I've requested a review from @angersson who knows more about this than I do. In the meanwhile, your PR failed a couple of our tests. Could you please help take a look? Below are some quick links to the error log.
Android Demo App
Ubuntu Makefile

Please ignore the Windows Bazel GPU failure. It's not related to this PR.

@dcirne
Copy link
Contributor Author

dcirne commented Dec 6, 2018

Hi @penpornk , thank you for the links, they helped in debugging. It seems that the tests failing was not related to the content of the PR itself, but to the version of flatbuffer, which seems somehow dependent on protobuf version 3.6.1.2. Since This PR originally used protobuf version 3.6.1, the file couldn't be found. (See snippet from the tests below.)

downloading https://github.com/google/flatbuffers/archive/1f5eae5d6a135ff6811724f6c57f911d1f46bb15.tar.gz
650
Cloning into 'tensorflow/contrib/makefile/downloads/protobuf'...
651
error: pathspec 'v3.6.1.2.tar.gz' did not match any file(s) known to git.
652
================================================================================
653
//tensorflow/tools/ci_build/builds:gen_makefile_out                      FAILED in 0.1s

I updated the PR to use protobuf version 3.6.1.2. I hope this will fix the tests.

@penpornk penpornk added the kokoro:force-run Tests on submitted change label Dec 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 6, 2018
@dcirne
Copy link
Contributor Author

dcirne commented Dec 6, 2018

@penpornk apologies for an earlier mistake. The commit I pushed about half an hour ago had an error in the URL to download version 3.6.1.2 of protobuf.

I have just corrected it and pushed a new commit. All the tests I could run on my side worked.

Protobuf has been failing due to its repository having dependencies. Downloading the .tar.gz form GitHub doesn't work because it does not bring its dependencies with it.

This PR introduces a function to clone a repository, initializa and update its submodules. The function is used to download protobuf as a repository dependency, rather than just a file.

The function is also implemented in a generic way, so if in the future other dependencies fall into the same situation, it can be reused.

With these changes, `tensorflow/workspace.bzl` goes back to using the commit sha in `PROTOBUF_URLS` and `PROTOBUF_STRIP_PREFIX`, rather than the tag of a release.

A command at the end of the `download_dependencies.sh` script was commented out. The comment says for it to be removed once protobug is fixed. Perhaps no longer necessary.
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

@dcirne Thank you for your fast response! I think the problem is the setting of PROTOBUF_SHA itself. I tried the command on both Linux and Mac, both gave me PROTOBUF_SHA=v3.6.1.2.tar.gz instead of an actual commit hash.

As for why 3.6.1.2, it turns out your tensorflow.bzl was out of date. The latest tensorflow.bzl is already using protobuf 3.6.1.2. Please sync to the latest commit. :)

tensorflow/contrib/makefile/download_dependencies.sh Outdated Show resolved Hide resolved
tensorflow/contrib/makefile/download_dependencies.sh Outdated Show resolved Hide resolved
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Forgot to highlight this line in my earlier review.

tensorflow/contrib/makefile/download_dependencies.sh Outdated Show resolved Hide resolved
@penpornk penpornk added the kokoro:force-run Tests on submitted change label Dec 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 6, 2018
@penpornk
Copy link
Member

penpornk commented Dec 6, 2018

@dcirne I just saw your new comment after submitting my review. Sorry for not addressing your comment earlier! I'm rerunning the tests.

`PROTOBUF_SHA` renamed to `PROTOBUF_TAG`. The variable is used to checkout to a specific tag after cloning the protobuf repository. The value is obtained by parsing the protobuf URL in the `workspace.bzl` file.
@dcirne
Copy link
Contributor Author

dcirne commented Dec 6, 2018

@penpornk thank you for the feedback. I have just pushed a new commit addressing all the points from your review.

@penpornk penpornk added the kokoro:force-run Tests on submitted change label Dec 6, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Dec 6, 2018
Copy link
Member

@penpornk penpornk left a comment

Choose a reason for hiding this comment

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

Thank you for the fixes! The changes look good to me. I'll go ahead and approve it. Hope the tests pass this time. :)

@penpornk
Copy link
Member

penpornk commented Dec 6, 2018

Only Windows Bazel and Windows Bazel GPU failed. These are existing failures. We are good to go. Thank you again for the PR!

@penpornk penpornk added ready to pull PR ready for merge process and removed awaiting review Pull request awaiting review labels Dec 6, 2018
@tensorflow-copybara tensorflow-copybara merged commit 5bedbfd into tensorflow:master Dec 6, 2018
tensorflow-copybara pushed a commit that referenced this pull request Dec 6, 2018
@dcirne dcirne deleted the protobuf_ios branch December 6, 2018 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants