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 build: Keep generated sources and Python runtime in the same directory #1586

Merged
merged 1 commit into from
May 26, 2016

Conversation

davidzchen
Copy link
Contributor

Users often encounter a Python import error when trying to build Python
protos if protobuf is installed locally on the machine. In this case,
Python ends up looking in the wrong directory when importing files (see
bazelbuild/bazel#1209 and tensorflow/tensorflow#2021). It seems that the
problem is caused by Python getting confused when there are Python
source files that are meant to be part of the same package but are
in separate directories.

Prior to #1233, the Bazel build setup would copy the Python
runtime sources and all generated sources for the builtin protos into
the root directory (assuming that the protobuf tree is vendored in a
google/protobuf directory).

With #1233, the two sets of sources are kept in their respective
directories but both src/ and python/ are added to the PYTHONPATH
using the new imports attribute of the Bazel Python rules. However,
both the runtime sources and the generated sources are under the same
package: google.protobuf, causing Python to become confused when
trying to import modules that are in the other directory.

This patch adds a workaround to the Bazel build to add a modified
version of the original internal_copied_filegroup macro to copy the
.proto files under src/ to python/ before building the
py_proto_library targets for the builtin protos. This ensures that the
generated sources for the builtin protos will be in the same directory
as the corresponding runtime sources.

This patch was tested with the following:

  • All Python tests in protobuf
  • All Python tests in tensorflow
  • All tests in Skydoc

@grpc-kokoro
Copy link

Thanks for your pull request. The automated tests will run as soon as one of the admins verifies this change is ok to test.

@davidzchen
Copy link
Contributor Author

@davidzchen
Copy link
Contributor Author

The first test failure are in jruby. For the python_cpp failure, it is unclear how this change would have caused it since this change is only in the Bazel build files.

@liujisi
Copy link
Contributor

liujisi commented May 24, 2016

LG

Let me know if the following will work:

  1. users submodule it as //google/protobuf
  2. import and bind it to //external
  3. import and put it under //third_party/protobuf
  4. the target in the protobuf repo itself.

@davidzchen davidzchen force-pushed the python branch 2 times, most recently from 27b37dd to 3d88872 Compare May 26, 2016 00:58
…rectory.

Users often encounter a Python import error when trying to build Python
protos if protobuf is installed locally on the machine. In this case,
Python ends up looking in the wrong directory when importing files (see
bazelbuild/bazel#1209 and tensorflow/tensorflow#2021). It seems that the
problem is caused by Python getting confused when there are Python
source files that are meant to be part of the same package but are
in separate directories.

Prior to protocolbuffers#1233, the Bazel build setup would copy the Python
runtime sources and all generated sources for the builtin protos into
the root directory (assuming that the protobuf tree is vendored in a
google/protobuf directory).

With protocolbuffers#1233, the two sets of sources are kept in their respective
directories but both `src/` and `python/` are added to the `PYTHONPATH`
using the new `imports` attribute of the Bazel Python rules. However,
both the runtime sources and the generated sources are under the same
package: `google.protobuf`, causing Python to become confused when
trying to import modules that are in the other directory.

This patch adds a workaround to the Bazel build to add a modified
version of the original `internal_copied_filegroup` macro to copy the
`.proto` files under `src/` to `python/` before building the
`py_proto_library` targets for the builtin protos. This ensures that the
generated sources for the builtin protos will be in the same directory
as the corresponding runtime sources.

This patch was tested with the following:
* All Python tests in protobuf
* All Python tests in tensorflow
* All tests in [Skydoc](https://github.com/bazelbuild/skydoc)
* Importing protobuf as `//google/protobuf`
* Importing and binding targets under `//external`
* Importing protobuf as `//third_party/protobuf`
@davidzchen
Copy link
Contributor Author

@pherl Verified with the additional cases you mentioned and updated commit message. PTAL

@liujisi
Copy link
Contributor

liujisi commented May 26, 2016

LG

@liujisi liujisi merged commit ed87c1f into protocolbuffers:master May 26, 2016
@davidzchen
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants