disable buidling/deployment of python wheels #583

anthrotype · 2017-08-08T15:30:25Z

Since this is now done in a separate https://github.com/google/brotli-wheels, it makes sense remove that from the upstream brotli's Travis and Appveyor setup.
We still run python setup.py test on both 2.7 and 3.6 on linux and windows, as well as on the system's python on macOS.

all the other python versions are being built and tested on https://github.com/google/brotli-wheels/blob/d571d63/appveyor.yml

…ilds All the other python versions for OSX are being built/tested on: https://github.com/google/brotli-wheels/blob/d571d63/.travis.yml Also, there's no need to build and deploy wheels here, as that's done in the separate repository.

…'depends' https://github.com/python/cpython/blob/v3.6.2/Lib/distutils/command/build_ext.py#L485-L500

anthrotype · 2017-08-08T17:01:34Z

I noticed that we were re-building the extension module three times before actually running the tests...

It turns out

the code that checks if the extension module was newer than its source files was not there in our implementation of build_ext.build_extension method (I just copied it from cpython's distutils)
there was a typo in the list of headers (a missing comma) which made it so that two headers' paths were combined into one, but the latter did not exist and thus distutils/setuptools would think the extension module was not up-to-date (as one of the dependencies was showing as missing), and would always run the built_ext command over and over.

if we run 'python setup.py built test', the setuptools 'test' command will forcibly re-run the build_ext subcommand because it wants to pass the --inplace option (it ignores whether it's up to date, just re-runs it all the time). with this we go from running built_ext twice, to running it only once per build

anthrotype · 2017-08-08T17:23:33Z

ok.. We went from running build_ext three times down to running it twice...
That's because running the test command will automatically run build_ext as a dependent subcommand, so we don't need build and test in a single setup.py invocation.

With the last commit we call the compiler only once (384f8b6), by simply doing python setup.py test.

anthrotype · 2017-08-08T17:27:41Z

linux python 2.7 build went down from 3m 40s to 2m 12s. nice

…target The 'develop' command is like 'install' in the sense that it modifies the user's python environment. The default make target should be less intrusive, i.e. just building the extension module in-place without modify anything in the user's environment. We don't need to tell make about the dependency between 'test' and 'build' target as that is baked in the `python setup.py test` command.

nicksay · 2017-08-08T17:34:48Z

setup.py

@@ -53,6 +55,21 @@ def get_source_files(self):
        return filenames

    def build_extension(self, ext):
+        if ext.sources is None or not isinstance(ext.sources, (list, tuple)):
+            from distutils.errors import DistutilsSetupError


I think it's okay to move this import to the top

nicksay · 2017-08-08T17:36:11Z

setup.py

@@ -15,6 +15,8 @@
    from distutils.core import Extension
    from distutils.core import setup
 from distutils.command.build_ext import build_ext
+from distutils.dep_util import newer_group


Let's try to import packages/modules instead of functions:
https://google.github.io/styleguide/pyguide.html#Imports

nicksay · 2017-08-08T17:39:17Z

setup.py

+
+        ext_path = self.get_ext_fullpath(ext.name)
+        depends = ext.sources + ext.depends
+        if not (self.force or newer_group(depends, ext_path, 'newer')):


What are your thoughts on "newer" vs the default "error"?

don't know.. I just copied from the default distutils build_ext module. I thought it'd be better to keep the default behavior. But I see that with "error" we could have avoided issues like the missing comma..

nicksay · 2017-08-08T17:40:29Z

python/README.md

@@ -24,7 +24,7 @@ able to edit the source files.

 For convenience, you may run the following commands from this directory:

-    $ make          # Deploy the module in "development mode"
+    $ make          # Build the module in-place


Maybe leave something about "development mode" here since it's described in the paragraph right before this?

oh I see now, sorry.. I can actually add an additional make develop, wdyt?

Yeah, that sounds good to me, though perhaps we should remove the references to "development mode" in the README if the in-place build is the default.

anthrotype · 2017-08-08T17:42:52Z

@nicksay FYI I have modified the Makefile in 44b5e66 so that the default make target does not call the develop command, which is I believe is too intrusive for a simple default build target.
It's good for developers who want to actually modify the code, but for the sake of testing the build it's better to not modify the user's python environment (e.g. say one does not have admin rights, develop command will fail, just like install).

Also, we don't need to tell make about the dependency between 'test' and 'build' targets as that is baked in the python setup.py test command as I explained above.

`make test` is good enough

This will work even if setuptools is not installed, which is unlikely nowadays but still our `setup.py` works with plain distutils, so we may well have our tests work without setuptools.

…'make test'

google#583 (comment)

anthrotype · 2017-08-08T18:26:59Z

@nicksay I added the make develop target and reworked the python/README.md so that the reference to setuptools development mode is still there but no longer at the top of the section.

I noticed also that the test command is setuptools only, and the brotli setup.py still works with plain distutils (setuptools is widely available in modern python distributions but is not strictly part of the standard library). So I decided to revert the make test command to calling python -m unittest discover... as it was before, for backward compatiblity's sake.

nicksay

Can you add the new "develop" make target as .PHONY at the top of the file? Otherwise, LGTM

remove 'tests' from .PHONY

We don't need to install custom python versions, we are using the pre-installed ones on Appveyor.

anthrotype · 2017-08-09T06:38:25Z

The last Travis failure is just random and unrelated, this is good to go now.

nicksay · 2017-08-09T17:29:25Z

Still LGTM 👍

* [appveyor] remove 'deploy' stage; only test python 2.7 and 3.6 all the other python versions are being built and tested on https://github.com/google/brotli-wheels/blob/d571d63/appveyor.yml * remove terrify submodule as not needed any more * [travis] just test py2.7 and 3.6 on linux; remove extra osx python builds All the other python versions for OSX are being built/tested on: https://github.com/google/brotli-wheels/blob/d571d63/.travis.yml Also, there's no need to build and deploy wheels here, as that's done in the separate repository. * [setup.py] only rebuild if dependency are newer; fix typo in list of 'depends' https://github.com/python/cpython/blob/v3.6.2/Lib/distutils/command/build_ext.py#L485-L500 * [ci] only run 'python setup.py test' if we run 'python setup.py built test', the setuptools 'test' command will forcibly re-run the build_ext subcommand because it wants to pass the --inplace option (it ignores whether it's up to date, just re-runs it all the time). with this we go from running built_ext twice, to running it only once per build * [Makefile] run 'build_ext --inplace' instead of 'develop' as default target The 'develop' command is like 'install' in the sense that it modifies the user's python environment. The default make target should be less intrusive, i.e. just building the extension module in-place without modify anything in the user's environment. We don't need to tell make about the dependency between 'test' and 'build' target as that is baked in the `python setup.py test` command. * [Makefile] add 'develop' target; remove unnecessary 'tests' target `make test` is good enough * [Makefile] `setup.py test` requires setuptools; run `python -m unittest` This will work even if setuptools is not installed, which is unlikely nowadays but still our `setup.py` works with plain distutils, so we may well have our tests work without setuptools. * [python/README.md] add ref to 'develop' target; remove 'tests', just 'make test' * [setup.py] import modules as per nicksay's comment google#583 (comment) * [Makefile] add 'develop' to .PHONY targets remove 'tests' from .PHONY * [appveyor] remove unused setup scripts We don't need to install custom python versions, we are using the pre-installed ones on Appveyor. * [appveyor] remove unneeded setup code

anthrotype added 4 commits August 8, 2017 16:07

[appveyor] remove 'deploy' stage; only test python 2.7 and 3.6

75aa483

all the other python versions are being built and tested on https://github.com/google/brotli-wheels/blob/d571d63/appveyor.yml

remove terrify submodule as not needed any more

f54b514

[setup.py] only rebuild if dependency are newer; fix typo in list of …

86d4b19

…'depends' https://github.com/python/cpython/blob/v3.6.2/Lib/distutils/command/build_ext.py#L485-L500

nicksay suggested changes Aug 8, 2017

View reviewed changes

anthrotype added 4 commits August 8, 2017 18:57

[Makefile] add 'develop' target; remove unnecessary 'tests' target

4e370f5

`make test` is good enough

[Makefile] setup.py test requires setuptools; run python -m unittest

0a3c14c

This will work even if setuptools is not installed, which is unlikely nowadays but still our `setup.py` works with plain distutils, so we may well have our tests work without setuptools.

[python/README.md] add ref to 'develop' target; remove 'tests', just …

47b4aab

…'make test'

[setup.py] import modules as per nicksay's comment

44f6d75

google#583 (comment)

nicksay approved these changes Aug 8, 2017

View reviewed changes

anthrotype added 3 commits August 8, 2017 22:45

[Makefile] add 'develop' to .PHONY targets

ab16a1b

remove 'tests' from .PHONY

[appveyor] remove unused setup scripts

70bb27b

We don't need to install custom python versions, we are using the pre-installed ones on Appveyor.

[appveyor] remove unneeded setup code

4add62c

eustas merged commit 4f455ca into google:master Aug 23, 2017

anthrotype mentioned this pull request Sep 4, 2017

Release executable #591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disable buidling/deployment of python wheels #583

disable buidling/deployment of python wheels #583

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

nicksay Aug 8, 2017

nicksay Aug 8, 2017

nicksay Aug 8, 2017

anthrotype Aug 8, 2017

nicksay Aug 8, 2017

anthrotype Aug 8, 2017

nicksay Aug 8, 2017

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

nicksay left a comment

anthrotype commented Aug 9, 2017 •

edited

Loading

nicksay commented Aug 9, 2017

disable buidling/deployment of python wheels #583

disable buidling/deployment of python wheels #583

Conversation

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

nicksay Aug 8, 2017

Choose a reason for hiding this comment

nicksay Aug 8, 2017

Choose a reason for hiding this comment

nicksay Aug 8, 2017

Choose a reason for hiding this comment

anthrotype Aug 8, 2017

Choose a reason for hiding this comment

nicksay Aug 8, 2017

Choose a reason for hiding this comment

anthrotype Aug 8, 2017

Choose a reason for hiding this comment

nicksay Aug 8, 2017

Choose a reason for hiding this comment

anthrotype commented Aug 8, 2017

anthrotype commented Aug 8, 2017

nicksay left a comment

Choose a reason for hiding this comment

anthrotype commented Aug 9, 2017 • edited Loading

nicksay commented Aug 9, 2017

anthrotype commented Aug 9, 2017 •

edited

Loading