Modify setup.py to build protos on Windows #723
Conversation
|
We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google. |
|
CLAs look good, thanks! |
setup.py
Outdated
| @@ -109,6 +114,9 @@ def run(self): | |||
| '"protobuf-compiler" and "libprotobuf-dev" packages.') | |||
| elif sys.platform == 'darwin': | |||
| print('On Mac, protobuf is often installed via homebrew.') | |||
| else: | |||
| print('On Windows, protoc should be installed and added ' | |||
| 'to the path.') | |||
There was a problem hiding this comment.
Maybe you could leave more of a bread crumb here for how the user should go about getting protoc for windows, even if it's just to point them at the protobuf website. The Linux and Mac strings at least mention which method is recommended to get protoc.
There was a problem hiding this comment.
Added a link to https://github.com/google/protobuf/releases
setup.py
Outdated
| if not self.protoc: | ||
| self.protoc = find_executable('protoc') | ||
| pc_path = os.path.dirname(find_executable('protoc')) | ||
| self.protodir = os.path.abspath(os.path.dirname(pc_path) + '/../lib') |
There was a problem hiding this comment.
Prefer using os.path.join instead of hard-coding path separators.
Also change yaml library to one that builds on Windows Also add some setup instructions for Windows
|
PTAL at the latest change. Sorry, I force pushed and overwrote the first commit (haven't worked with pull requests before). |
setup.py
Outdated
| if not self.protoc: | ||
| self.protoc = find_executable('protoc') | ||
| pc_path = os.path.dirname(find_executable('protoc')) | ||
| self.protodir = os.path.abspath(os.path.join(os.path.dirname(pc_path), '../lib')) |
There was a problem hiding this comment.
'../' should probably be os.path.pardir instead.
|
PTAL |
|
Try re-running that unit test after rebasing on master. I made a change recently that should have fixed its flakiness. |
| self.protoc = 'protoc' | ||
|
|
||
| self.protodir = os.path.join(prefix, 'include') | ||
| if not self.protoc: |
There was a problem hiding this comment.
With these changes, Mac and Linux will fallback to using the Windows logic if maybe_protoc is not found. This doesn't seem right. For example, the proto dir should remain as os.path.join(prefix, 'include') on Mac/Linux instead of using /usr/lib/.
It seems like the Mac/Linux logic should remain unchanged by this PR, so I suggest restructuring the flow control to something like:
if prefix:
# Linux/Mac.
maybe_protoc = os.path.join(prefix, 'bin', 'protoc')
if os.path.isfile(maybe_protoc) and os.access(maybe_protoc, os.X_OK):
self.protoc = maybe_protoc
else:
print('Warning: protoc not found at %s' % maybe_protoc)
print('setup will attempt to run protoc with no prefix.')
self.protoc = 'protoc'
self.protodir = os.path.join(prefix, 'include')
else:
# Windows.
...| if not self.protoc: | ||
| self.protoc = find_executable('protoc') | ||
| pc_path = os.path.dirname(find_executable('protoc')) | ||
| self.protodir = os.path.abspath(os.path.join(os.path.dirname(pc_path), os.path.pardir, 'lib')) |
There was a problem hiding this comment.
I'm not familiar with protos on Windows, but this looks a bit suspicious, in that you're going up two directory levels and assuming that lib/ is there.
If you expect lib/ in a certain place on Windows, consider hardcoding that path. This is similar to what we do for Mac/Linux where we check /usr/local/include/ on Mac and /usr/include/ on Linux. If needed, you can get the drive path using os.path.splitdrive(self.protoc)[0].
If you actually want to go up two directories then this should be cleaned up as:
self.protoc = find_executable('protoc')
self.protodir = os.path.join(
os.path.dirname(os.path.dirname(os.path.dirname(self.protoc))), 'lib')
arsharma1
dismissed
jettisonjoe’s stale review
This review is a year old and hasn't been committed.
Also change yaml library to one that builds on Windows. I've test python setup.py install on Windows 10 and Linux. Let me know if there's anything else you'd like me to test. Thanks!
This change is