-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 build for Bazel 0.25 #2243
Fix build for Bazel 0.25 #2243
Conversation
Summary: Fixes #2181. Thankfully, the `rules_webtesting` scene has gotten a bit saner, requiring fewer explicitly defined transitive dependencies, though it looks like the ones that we do have need to be manually kept in sync (e.g., `bazel_skylib`). Test Plan: Running `bazel build //...` and `bazel test //...` works in Bazel 0.25.2 in both Python 2 and Python 3. wchargin-branch: bazel-0.25
) | ||
|
||
load("@io_bazel_rules_webtesting//web:repositories.bzl", "web_test_repositories") | ||
web_test_repositories() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I think I am slowly getting this. Just wondering, it looks like there is some logic to load skylib inside.
https://github.com/bazelbuild/rules_webtesting/blob/14137ff84f4b2015ed2f815c59df0c1ef9a227e0/web/repositories.bzl#L42-L43
Do we define skylib dependency only because we use versions in it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. Removing the bazel_skylib
archive and the versions
check yields
a working state. Keeping bazel_skylib
at a version distinct from that
of rules_webtesting
causes problems.
It is also technically sufficient to fetch @bazel_skylib
only
indirectly through rules_webtesting
, due to the definition that you
link to. But this seems like a bad idea (include-what-you-use and all).
"https://mirror.bazel.build/github.com/tensorflow/tensorflow/archive/6ef428bd6e83b0930266bf922eaa2f4a60e8328a.tar.gz", # 2018-12-06 | ||
"https://github.com/tensorflow/tensorflow/archive/6ef428bd6e83b0930266bf922eaa2f4a60e8328a.tar.gz", | ||
"https://mirror.bazel.build/github.com/tensorflow/tensorflow/archive/2243bd6ba9b36d43dbd5c0ede313853f187f5dce.tar.gz", # 2019-03-26 | ||
"https://github.com/tensorflow/tensorflow/archive/2243bd6ba9b36d43dbd5c0ede313853f187f5dce.tar.gz", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this require an update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two patches to TensorFlow Bazel files:
Each of these fixes an --incompatible_*
flag that otherwise hits us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for doing this!
Summary:
Fixes #2181. Thankfully, the
rules_webtesting
scene has gotten a bitsaner, requiring fewer explicitly defined transitive dependencies,
though it looks like the ones that we do have need to be manually kept
in sync (e.g.,
bazel_skylib
).Test Plan:
Running
bazel build //...
andbazel test //...
works in Bazel 0.25.2in both Python 2 and Python 3.
wchargin-branch: bazel-0.25