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

bazel version check should be in tf_workspace() #4975

Closed
jart opened this issue Oct 14, 2016 · 8 comments
Closed

bazel version check should be in tf_workspace() #4975

jart opened this issue Oct 14, 2016 · 8 comments
Labels
stat:contribution welcome Status - Contributions welcome type:feature Feature requests

Comments

@jart
Copy link
Contributor

jart commented Oct 14, 2016

@ic suggested in #4458 to have ./configure check the Bazel version. This would probably save time for a lot of people. It should be relatively straightforward to implement, because even if Bazel is compiled from git, it still displays a version tag.

$ bazel version
Build label: 0.3.2-2016-10-14 (@183147e)
Build target: bazel-out/local-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Oct 14 21:34:37 2016 (1476480877)
Build timestamp: 1476480877
Build timestamp as int: 1476480877

@martinwicke what version do we currently support? Bazel ≥0.3.1?

@martinwicke
Copy link
Member

bazel should already check the bazel version, but maybe that check is broken, or we are not supporting the minimum version we pretend to support there? See tensorflow.bzl:check_version and uses.

@jart
Copy link
Contributor Author

jart commented Oct 15, 2016

Oh terrific. It's probably better to have the version check in Skylark. Although we might want to move that into the tf_workspace() function so it gets enforced on dependent repositories as well.

@jart jart closed this as completed Oct 15, 2016
@martinwicke
Copy link
Member

Ah, yes. That would be much better, otherwise dependent repos have to
remember to make their own (which they might have to anyway, but at least
we still have the floor)
On Fri, Oct 14, 2016 at 19:07 Justine Tunney notifications@github.com
wrote:

Oh terrific. It's probably better to have the version check in Skylark.
Although we might want to move that into the tf_workspace() function so it
gets enforced on dependent repositories as well.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#4975 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAjO_ePLzwDJFCn8U69O4ZUiYv_E2DrZks5q0DVMgaJpZM4KXhrT
.

@ic
Copy link
Contributor

ic commented Oct 16, 2016

All this sounds terrific. The check trickling down to dependencies would be great, yes!

but maybe that check is broken

If so, would this issue better be open or renamed? As far as I can test myself or read about it, there are some issues related to either Bazel or Protobuf versioning. I can dig out and link recent issues, if this helps.

I am willing to spend time on the issue, although I am more interested in the Makefile and know little about Bazel right now (just starting to study it).

@jart jart changed the title configure should check bazel version bazel version check should be in tf_workspace() Oct 16, 2016
@jart jart added enhancement stat:contribution welcome Status - Contributions welcome labels Oct 16, 2016
@jart jart removed their assignment Oct 16, 2016
@jart
Copy link
Contributor Author

jart commented Oct 16, 2016

You're right @ic. I've renamed and reopened this issue. I'm not sure if you're volunteering, but if you write up a PR, mention me in the description and I'll be happy to review it. If not, then I can probably take care of this when I get time.

@jart jart reopened this Oct 16, 2016
@ic
Copy link
Contributor

ic commented Oct 16, 2016

I will look into it---got bitten a few times already by this issue. Two issues to mitigate what to expect from me: I have to work on another part completely unrelated (quantization), and I am just started on Bazel. Anyway, understood on the mention!

@ic
Copy link
Contributor

ic commented Oct 17, 2016

Looking a bit into it, would the problem occur in WORKSPACE:

# Specify the minimum required bazel version.
load("//tensorflow:tensorflow.bzl", "check_version")
check_version("0.3.0")

There were quite a few issues/discussions recently about Bazel 0.3.1. I had a look again, but the data is piling up and getting hard to drill.

Would it simply be that the minimum requirement is 0.3.1 now? Note that the current master (f794cd3) builds ok without GPU support, and Makefile-related pieces build too. However the code for quantization does not build (the part under contrib).

@ic
Copy link
Contributor

ic commented Oct 25, 2016

After pulling today, WORKSPACE blocked any progress, requiring for Bazel 0.3.2. The version check works well! I guess the issue we had earlier was just a missing version bump (it was 0.3.0 for a while).

@jart I think this issue can be closed now; thank you.

@aselle aselle added type:feature Feature requests and removed enhancement labels Feb 9, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat:contribution welcome Status - Contributions welcome type:feature Feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants