Make curl forward/backward compatible #7165

Merged
merged 2 commits into from Feb 11, 2017

Projects

None yet

7 participants

@kchodorow
Contributor
kchodorow commented Jan 31, 2017 edited

I'm working on fixing bazelbuild/bazel#1262, which will change the way Bazel organizes the execution root. The curl library needs to use ../curl as an include prefix in new versions of Bazel and external/curl in old versions.

This patch should be forward/backward compatible.

@kchodorow kchodorow requested a review from jart Jan 31, 2017
@tensorflow-jenkins
Collaborator

Can one of the admins verify this patch?

@jart jart was assigned by rmlarsen Jan 31, 2017
tensorflow/workspace.bzl
@@ -52,6 +52,30 @@ temp_workaround_http_archive = repository_rule(
"strip_prefix": attr.string(default = ""),
})
+def _repos_are_siblings():
+ return Label("@foo//bar").workspace_root.startswith("../")
@kchodorow
kchodorow Jan 31, 2017 Contributor

Good call. I'll check that and let you know. Added it to my tracking doc.

@kchodorow
kchodorow Feb 1, 2017 Contributor

Created bazelbuild/rules_closure#184 to fix this, btw.

tensorflow/workspace.bzl
+def _repos_are_siblings():
+ return Label("@foo//bar").workspace_root.startswith("../")
+
+def _curl_repo_impl(ctx):
@jart
jart Jan 31, 2017 Member

Would it be possible to refactor this to use temp_workaround_http_archive in workspace.bzl?

@kchodorow
kchodorow Jan 31, 2017 Contributor

Done.

@googlebot googlebot added the cla: yes label Jan 31, 2017
@rmlarsen
Member
rmlarsen commented Feb 1, 2017

@tensorflow-jenkins test this please

@rmlarsen
Member
rmlarsen commented Feb 1, 2017

@jart is this good to go?

@jart
jart approved these changes Feb 1, 2017 View changes
@jart
Member
jart commented Feb 1, 2017

My name is Justine Tunney and I approve this pull request.

@jart jart removed the awaiting review label Feb 1, 2017
@rmlarsen
Member
rmlarsen commented Feb 2, 2017

@tensorflow-jenkins test this please

@kchodorow
Contributor

I'm having trouble getting these tests to pass, even without my changes. Is there anything in particular I need to set up to get, say, Linux CPU Tests working? I've got a vanilla TF setup without my changes and I'm getting (for example, with bazel test //tensorflow/contrib/factorization/examples:mnist):

exec ${PAGER:-/usr/bin/less} "$0" || exit 1
-----------------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/kchodorow/.cache/bazel/_bazel_kchodorow/d39e1471d317a89f747169f52aae8653/execroot/clean_tensorflow/bazel-out/local-opt/bin/tensorflow/contrib/factorization/examples/mnist.runfiles/org_tensorflow/tensorflow/contrib/factorization/examples/mnist.py", line 36, in <module>
    import tensorflow as tf
  File "/home/kchodorow/.cache/bazel/_bazel_kchodorow/d39e1471d317a89f747169f52aae8653/execroot/clean_tensorflow/bazel-out/local-opt/bin/tensorflow/contrib/factorization/examples/mnist.runfiles/org_tensorflow/tensorflow/__init__.py", line 24, in <module>
    from tensorflow.python import *
  File "/home/kchodorow/.cache/bazel/_bazel_kchodorow/d39e1471d317a89f747169f52aae8653/execroot/clean_tensorflow/bazel-out/local-opt/bin/tensorflow/contrib/factorization/examples/mnist.runfiles/org_tensorflow/tensorflow/python/__init__.py", line 124, in <module>
    from tensorflow.python.platform import test
  File "/home/kchodorow/.cache/bazel/_bazel_kchodorow/d39e1471d317a89f747169f52aae8653/execroot/clean_tensorflow/bazel-out/local-opt/bin/tensorflow/contrib/factorization/examples/mnist.runfiles/org_tensorflow/tensorflow/python/platform/test.py", line 83, in <module>
    import mock                # pylint: disable=g-import-not-at-top,unused-import
ImportError: No module named mock
@rmlarsen
Member
rmlarsen commented Feb 2, 2017

@kchodorow: I notice that this import is conditional on the version:

https://github.com/tensorflow/tensorflow/blob/master/tensorflow/python/platform/test.py#L82

Could this check be wrong?

@kchodorow
Contributor

Ah, I didn't have mock installed! sudo pip install mock made it pass. Unfortunately, now it passes for me locally both with and without my change (gah!). Is there a way to see the Bazel tests logs Jenkins generates (so I can see what, specifically, made these things fail)?

@rmlarsen
Member
rmlarsen commented Feb 2, 2017 edited

@kchodorow Click on "Console Output" link for the tests here:

https://ci.tensorflow.org/job/tensorflow-pull-requests-multijob/3566/

For the full log, click on the "Full Log" link at the top of the individual console output pages. They are very verbose, but searching for "Failure" usually does the trick.

@jart
jart approved these changes Feb 8, 2017 View changes
@jart
Member
jart commented Feb 8, 2017

Mr. Jenkins test this please

kchodorow added some commits Jan 31, 2017
@kchodorow kchodorow Update proto library reference and make curl forward/backward compatible
I'm working on fixing bazelbuild/bazel#1681, which
will change the way Bazel organizes the execution root.  This has two
implications for tensorflow:

* The protobuf library needs to be updated to a version that works with
old/new versions of Bazel (older versions won't be able to find
external protos).
* The curl library needs to use ../curl as an include prefix in new
versions of Bazel and external/curl in old versions.

This patch should be forward/backward compatible.
7689ef2
@kchodorow kchodorow Use temp_http_archive
5bc7333
@kchodorow kchodorow changed the title from Update proto library reference and make curl forward/backward compatible to Make curl forward/backward compatible Feb 9, 2017
@kchodorow
Contributor

Okay, rebased my change on top of @jhseu's fix, so now it only updates the curl library. Could you try triggering the Jenkins build again (hopefully for the last time)?

@jhseu
Member
jhseu commented Feb 9, 2017

Jenkins, test this please

@kchodorow
Contributor

Horay!!!!

@kchodorow
Contributor

Friendly ping: could someone merge this in when you get a chance?

@kchodorow kchodorow referenced this pull request in bazelbuild/bazel Feb 10, 2017
Closed

Action cache is not working in some cases #2490

@gunan gunan merged commit 1225437 into tensorflow:master Feb 11, 2017

11 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux CPU Tests Makefile SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed
@kchodorow kchodorow referenced this pull request in tensorflow/serving Feb 13, 2017
Merged

Updated tensorflow submodule #321

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