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

Attempt at making package builds **seem** like "setup.py develop" #2698

Closed
wants to merge 3 commits into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Nov 7, 2016

@jonparrott Please weigh in here. I am mostly happy with this but unclear on a few sharp corners.

Things I wish were better:

  • Having to hard-code package deps in tox.ini (would be nice to install the deps from setup.py without installing the source
  • Having to fake the metadata for google-cloud-core (since we use pkg_resources.get_distribution('google-cloud-core') for the user agent)
  • Hack to make __path__ in Python 3 look like it does in Python 2 (unclear why the pep420 bool is in place in setuptools)

@dhermes dhermes added packaging do not merge Indicates a pull request not ready for merge, due to either quality or timing. labels Nov 7, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 7, 2016
@theacodes
Copy link
Contributor

I kind of don't like the idea of this, but if you really feel like you need it then go ahead and merge.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 7, 2016

@jonparrott Here is the problem I am trying to solve:

  • If local deps change, tox does not re-install them, which leads to inconsistent envs and forces tox -e ${ENV} --recreate any time core is touched

@theacodes
Copy link
Contributor

How big is the cost of tox --recreate? wheelcache should help significantly, unless for some reason some dependency can't be wheeled?

@dhermes
Copy link
Contributor Author

dhermes commented Nov 7, 2016

@jonparrott On my old-ish laptop with 16GB RAM the cost is prohibitive (it triples runtime):

$ cd ${GIT_ROOT}/datastore
$ rm -fr .tox
$ time tox -e py27
real    0m11.698s
user    0m9.456s
sys     0m0.804s
$ time tox -e py27
real    0m3.815s
user    0m3.412s
sys     0m0.344s
$ time tox -e py27
real    0m3.847s
user    0m3.448s
sys     0m0.308s
$ time tox -e py27 --recreate
real    0m9.772s
user    0m8.836s
sys     0m0.864s
$ time tox -e py27 --recreate
real    0m10.145s
user    0m9.184s
sys     0m0.796s

The whole point of unit tests is that they can be run during development, and 10s runs are not dev-user-friendly

TBH, 3.8s runs are also not dev-user-friendly, especially when the fresh sdist on every tox run eats most of that. Without tox:

$ source .tox/py27/bin/activate
(py27) $ time .tox/py27/bin/py.test unit_tests
real    0m0.685s
user    0m0.644s
sys     0m0.036s

For comparison, here are the times on this branch:

$ rm -fr .tox
$ time tox -e py27
real    0m6.930s
user    0m6.344s
sys     0m0.504s
$ time tox -e py27
real    0m2.079s
user    0m1.908s
sys     0m0.140s
$ time tox -e py27
real    0m2.196s
user    0m1.952s
sys     0m0.208s

@theacodes
Copy link
Contributor

theacodes commented Nov 7, 2016

Alrighty. I guess I can live with it until setuptools can get things fixed.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 7, 2016

Thanks for weighing in. FWIW I'm not happy about this hack, but I'm even less happy about the Python ecosystem support for namespace packages.

@tseaver Would love to hear what you think

@theacodes
Copy link
Contributor

@dhermes please follow pypa/setuptools#250 and chime in if their solution doesn't seem to fix the issues here.

@dhermes
Copy link
Contributor Author

dhermes commented Nov 8, 2016

OK this has now been rebased on top of the simple fix in #2705, but I'd still like to avoid the massive build-time penalty if possible.

@tseaver PTAL

@daspecster
Copy link
Contributor

@jonparrott @dhermes @tseaver so it looks like this will never be fixed for Python 2.7 or possibly even 3.4.

Limiting support for this to 3.5+ only seems like a reasonable decision to me.

It seems like not having this PR, or something like it, could be a barrier to contributors.

@dhermes dhermes closed this Dec 28, 2016
@dhermes dhermes deleted the pth-attempt branch December 28, 2016 21:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing. packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants