Skip to content

Commit

Permalink
Fix usage of six in //:protobuf_python rule and add import (#6310)
Browse files Browse the repository at this point in the history 
* Fix reference to six in //:protobuf_python rule

* Add six to protobuf_deps.bzl

* Use six archive directly as repo @six
  • Loading branch information
@aaliddell @anandolee
aaliddell authored and anandolee committed Jul 15, 2019
1 parent f2cfe2c commit a74c43b
Show file tree
Hide file tree
Showing 3 changed files with 10 additions and 7 deletions.
2 changes: 1 addition & 1 deletion BUILD
View file
Expand Up @@ -911,7 +911,7 @@ py_proto_library(
py_extra_srcs = glob(["python/**/__init__.py"]),
py_libs = [
":python_srcs",
"//external:six",
"@six//:six",
],
srcs_version = "PY2AND3",
visibility = ["//visibility:public"],
Expand Down
7 changes: 1 addition & 6 deletions WORKSPACE
View file
Expand Up @@ -13,7 +13,7 @@ new_local_repository(
)

http_archive(
name = "six_archive",
name = "six",
build_file = "@//:six.BUILD",
sha256 = "105f8d68616f8248e24bf0e9372ef04d3cc10104f1980f54d57b2ce73a5ad56a",
urls = ["https://pypi.python.org/packages/source/s/six/six-1.10.0.tar.gz#md5=34eed507548117b2ab523ab14b2f8b55"],
Expand Down Expand Up @@ -41,11 +41,6 @@ bind(
actual = "@submodule_gmock//:gtest_main",
)

bind(
name = "six",
actual = "@six_archive//:six",
)

maven_jar(
name = "guava_maven",
artifact = "com.google.guava:guava:18.0",
Expand Down
8 changes: 8 additions & 0 deletions protobuf_deps.bzl
View file
Expand Up @@ -13,3 +13,11 @@ def protobuf_deps():
strip_prefix = "zlib-1.2.11",
urls = ["https://zlib.net/zlib-1.2.11.tar.gz"],
)

if "six" not in native.existing_rules():
http_archive(
name = "six",
build_file = "@//:six.BUILD",

This comment has been minimized.

Sign in to view

Copy link
@rogerhub

rogerhub Jul 21, 2019

Contributor

Shouldn't this be @com_google_protobuf//:six.BUILD? This file is meant to be used in consumer WORKSPACE files as well.

For example, this fails for me:

$ git clone https://github.com/protocolbuffers/protobuf
$ cd protobuf/example
$ bazel build @com_google_protobuf//:protobuf_python
...
ERROR: An error occurred during the fetch of repository 'six':
   Traceback (most recent call last):
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/41ace61881c15f8317809cdaca0280b5/external/bazel_tools/tools/build_defs/repo/http.bzl", line 57
                workspace_and_buildfile(ctx)
        File "/home/ubuntu/.cache/bazel/_bazel_ubuntu/41ace61881c15f8317809cdaca0280b5/external/bazel_tools/tools/build_defs/repo/utils.bzl", line 61, in workspace_and_buildfile
                ctx.symlink(ctx.attr.build_file, "BUILD.bazel")
Not a regular file: /home/ubuntu/protobuf/examples/six.BUILD
...

$ bazel version
Build label: 0.28.1
Build target: bazel-out/k8-opt/bin/src/main/java/com/google/devtools/build/lib/bazel/BazelServer_deploy.jar
Build time: Fri Jul 19 15:19:51 2019 (1563549591)
Build timestamp: 1563549591
Build timestamp as int: 1563549591

This comment has been minimized.

Sign in to view

Copy link
@aaliddell

aaliddell Jul 21, 2019

Author Contributor

Yep, that seems wrong. Which is odd, as I tested importing as an external repo and had no issues, but perhaps six was defined elsewhere

This comment has been minimized.

Sign in to view

Copy link
@aaliddell

aaliddell Jul 21, 2019

Author Contributor

Added commit in bd46f7f17bd17c03e17119421f155a771078fb43 as part of #6391
sha256 = "105f8d68616f8248e24bf0e9372ef04d3cc10104f1980f54d57b2ce73a5ad56a",
urls = ["https://pypi.python.org/packages/source/s/six/six-1.10.0.tar.gz#md5=34eed507548117b2ab523ab14b2f8b55"],
)

6 comments on commit a74c43b

@laszlocsomor
Copy link
Contributor

@laszlocsomor laszlocsomor commented on a74c43b Jul 16, 2019 •
edited

Choose a reason for hiding this comment

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

FYI this commit breaks //:py_generator_test on Linux with Bazel 0.28.0

At this commit:

  $ bazel --version
bazel 0.28.0

  $ bazel test  //:py_generator_test --test_output=errors
(...)
INFO: From Testing //:py_generator_test:
==================== Test output for //:py_generator_test:
Traceback (most recent call last):
  File "/usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/01c7a16cdc16c48290b25e928dfd648e/sandbox/linux-sandbox/264/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/py_generator_test.runfiles/com_google_protobuf/python/google/protobuf/internal/generator_test.py", line 49, in <module>
    from google.protobuf.internal import test_bad_identifiers_pb2
  File "/usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/01c7a16cdc16c48290b25e928dfd648e/sandbox/linux-sandbox/264/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/py_generator_test.runfiles/com_google_protobuf/python/google/protobuf/internal/test_bad_identifiers_pb2.py", line 7, in <module>
    from google.protobuf import descriptor as _descriptor
  File "/usr/local/google/home/laszlocsomor/.cache/bazel/_bazel_laszlocsomor/01c7a16cdc16c48290b25e928dfd648e/sandbox/linux-sandbox/264/execroot/com_google_protobuf/bazel-out/k8-fastbuild/bin/py_generator_test.runfiles/com_google_protobuf/python/google/protobuf/descriptor.py", line 94, in <module>
    class DescriptorBase(six.with_metaclass(DescriptorMetaclass)):
AttributeError: module 'six' has no attribute 'with_metaclass'
================================================================================

At parent commit (f2cfe2c), this works.

@aaliddell
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is with_metaclass a function that is only in newer releases of six? If so, perhaps the current 1.10.0 should be bumped to 1.12.0, provided it's not going to cause issues elsewhere.

@laszlocsomor
Copy link
Contributor

Choose a reason for hiding this comment

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

SGTM, but I don't know.

(I'm also not a Protobuf maintainer.)

@aaliddell
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's curious this wasn't picked up by CI, but perhaps that target isn't used. Seeing as comments on commits tend to get lost in the abyss, it might be worth creating an issue instead.

@aaliddell
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, so it's not the version that's the problem, it's because of the old-favourite legacy_create_init flag. Sorting a PR now

@aaliddell
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #6391

Please sign in to comment.