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.

Sign up for GitHub

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

Modify setup.py to build protos on Windows #723

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Modify setup.py to build protos on Windows #723

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d7e3351
Modify setup.py to build protos on Windows
daiweili Feb 15, 2018
8756f1c
Change .. to os.path.pardir
daiweili Feb 23, 2018
b938700
Merge remote-tracking branch 'upstream/master'
Jul 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,13 +58,13 @@ when there are changes upstream after your PR was created.
We generally tend toward squashing commits on merge rather than filling our git
log with merge commits. Sometimes, however, especially for large, sweeping
changes, it makes sense to preserve multiple commits in the history. If you're
authoring such a PR, consider using a rebase with squashes and fixups to reduce
authoring such a PR, consider using a rebase with squashes and fixups to reduce
the commit history down to a commit per salient change.


### Code Standards
For a smooth code review, it helps to make sure your code adheres to standards,
conventions, and design goals for OpenHTF. A best-effort attempt to understand
conventions, and design goals for OpenHTF. A best-effort attempt to understand
and meet these standards before requesting code review can go a long way towards
making the review process as fast and painless as possible.

Expand All @@ -73,7 +73,7 @@ making the review process as fast and painless as possible.
OpenHTF's Python code follows the
[Google Python Style Guide](https://google.github.io/styleguide/pyguide.html).
We provide a `pylintrc` file at the top level of our repo that you can use with
[pylint](https://www.pylint.org/) to lint your Python code. We expect the
[pylint](https://www.pylint.org/) to lint your Python code. We expect the
codebase to produce zero pylint warnings, and we allow the use of explicit
`pylint: disable=...` comments in certain cases where it makes sense.

Expand Down Expand Up @@ -173,8 +173,7 @@ fits in based on what it does.
| |
| |
| '-> util
|
| Generic utility functions and miscellaneous tools.
|
| The contents of this submodule should be general enough to be
| usable outside of OpenHTF, meaning it should not be dependent
| on other code in the OpenHTF package.
Expand Down Expand Up @@ -239,7 +238,7 @@ python setup.py develop
```

### MacOS
We will use [Homebrew](https://brew.sh/) to install our dependencies and Pip to set up the virtualenv. We recommend installing [Xcode](https://developer.apple.com/xcode/) first as the GCC compiler will be needed for both; however, other GCC compilers not associated with Xcode may work just as well.
We will use [Homebrew](https://brew.sh/) to install our dependencies and Pip to set up the virtualenv. We recommend installing [Xcode](https://developer.apple.com/xcode/) first as the GCC compiler will be needed for both; however, other GCC compilers not associated with Xcode may work just as well.

```bash
# Install dependencies.
Expand Down Expand Up @@ -271,7 +270,32 @@ virtualenv venv
python setup.py develop
```

If you're having issues with the python setup, it's possible that the problem is due to El Capitan not including ssl headers. This [link](http://adarsh.io/bundler-failing-on-el-capitan/) may help you in that regard.
If you're having issues with the python setup, it's possible that the problem is due to El Capitan not including ssl headers. This [link](http://adarsh.io/bundler-failing-on-el-capitan/) may help you in that regard.

### Windows

Windows is similar to other platforms. It requires Pip to set up the virutalenv and protoc to be in the path. A pre-built binary can be downloaded from
[link](https://github.com/google/protobuf/releases).

```
# Clone into the repo.
git clone https://github.com/google/openhtf.git

# Install virtualenv via pip.
pip install virtualenv

# Change to the openhtf directory.
cd openhtf

# Create a new virtualenv.
virtualenv venv

# Activate the new virtualenv.
venv/Scripts/activate

# Install openhtf into the virtualenv in dev mode.
python setup.py develop
```

## Web Frontend Development
OpenHTF ships with a built-in web gui found in the `openhtf.output.web_gui` module.
Expand Down
31 changes: 17 additions & 14 deletions setup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
from distutils.command.build import build
from distutils.command.clean import clean
from distutils.cmd import Command
from distutils.spawn import find_executable
from setuptools import find_packages
from setuptools import setup
from setuptools.command.test import test
Expand Down Expand Up @@ -51,7 +52,6 @@ class BuildProtoCommand(Command):
('outdir=', 'o', 'Where to output .py files.')]

def initialize_options(self):
self.skip_proto = False
try:
prefix = subprocess.check_output(
'pkg-config --variable prefix protobuf'.split()).strip().decode('utf-8')
Expand All @@ -63,30 +63,29 @@ def initialize_options(self):
# Default to /usr/local for Homebrew
prefix = '/usr/local'
else:
print('Warning: mfg-inspector output is not fully implemented for '
'Windows. OpenHTF will be installed without it.')
self.skip_proto = True
prefix = None

maybe_protoc = os.path.join(prefix, 'bin', 'protoc')
if os.path.isfile(maybe_protoc) and os.access(maybe_protoc, os.X_OK):
self.protoc = None
if prefix:
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:
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')
if not self.protoc:
Copy link
Contributor

Choose a reason for hiding this comment

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

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.
  ...

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'))
Copy link
Contributor

Choose a reason for hiding this comment

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

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')

else:
self.protodir = os.path.join(prefix, 'include')
self.indir = os.getcwd()
self.outdir = os.getcwd()

def finalize_options(self):
pass

def run(self):
if self.skip_proto:
print('Skipping building protocol buffers.')
return

# Build regular proto files.
protos = glob.glob(
os.path.join(self.indir, 'openhtf', 'output', 'proto', '*.proto'))
Expand All @@ -109,6 +108,10 @@ 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. Pre-built binaries can be found at '
'https://github.com/google/protobuf/releases')
raise
except subprocess.CalledProcessError:
print('Could not build proto files.')
Expand Down