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

Groundwork for supporting PEP-518/pyproject.toml #577

Closed
wants to merge 4 commits into from

Conversation

ntninja
Copy link

@ntninja ntninja commented Aug 14, 2017

This PR currently adds two new options:

  • install_setuptools (defaulting to True) to the per-testenv settings
  • venvsdist (defaulting to False) to the global tox settings

When venvsdist is True and a sdist is to be created then tox will provision a new virtual environment and run ./setup.py sdist in there. All settings for that environment (except changedir, commands & list_dependencies_command) can be tweaked using the new toxenv:sdist configuration section.

(All new introduced names are ofc subject to change at your leisure.)

.

Here's a quick checklist of things that will be done once the current set of changes has been ACK'ed:

  • Make sure to include one or more tests for your change;
  • if an enhancement PR please create docs and at best an example
  • Add yourself to CONTRIBUTORS;
  • make a descriptive Pull Request text (it will be used for changelog)

@gaborbernat
Copy link
Member

@Alexander255 it's hard to accept this without any unit tests to demonstrate the new features

@ntninja
Copy link
Author

ntninja commented Aug 14, 2017

I didn't ask you to, but I first want a confirmation that these changes will be accepted in principle (or comments on how to improve the current design), before I go through the trouble of writing tests & documentation.

Copy link
Member

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

I would say looks good at a high level; but would want to not have groundwork but instead one PR adding the support as a whole. Will review and support with the testing, so go ahead and add tests.

tox/config.py Outdated
@@ -809,10 +840,11 @@ def _list_section_factors(self, section):
factors.update(*mapcat(_split_factor_expr, exprs))
return factors

def make_envconfig(self, name, section, subs, config, replace=True):
def make_envconfig(self, name, section, subs, config, replace=True,
Copy link
Member

Choose a reason for hiding this comment

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

note that the list here is globally change-able; you probably want to default to None, and if it's none then locally swap to "testenv"

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

tox/venv.py Outdated
@@ -178,7 +186,7 @@ def update(self, action):
self.envconfig.deps, v)

def _getliveconfig(self):
python = self.envconfig.python_info.executable
python = str(self.envconfig.python_info.executable)
Copy link
Member

Choose a reason for hiding this comment

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

care to detail why you need to wrap?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure why this was necessary, but I cannot reproduce the test failure that had been fixed during development of this comment. Thanks for pointing this out!

@obestwalter obestwalter added the feature:new something does not exist yet, but should label Aug 22, 2017
@ntninja ntninja force-pushed the master branch 2 times, most recently from 6a4d3c8 to 9f3df33 Compare August 23, 2017 20:23
@obestwalter obestwalter added the needs:work for PRs: not quite there and needs some changes label Sep 4, 2017
@obestwalter
Copy link
Member

obestwalter commented Sep 9, 2017

I have been thinkiing a lot recently about how we can make tox live long and prosper and IMHO extending the core to provide more hooks to make extensions like this possible is the way to go. That way the core won't get bloated and there is lots of room to experiment and extend without having to wait for PRs merged. I will concentrate on making that possible and so I would not merge this PR but rather work towards that functionality like this can be implemented fully as a plugin.

The essence of this PR could then be used to implement this functionality.

@gaborbernat
Copy link
Member

@Alexander255 I've recently got into the details of PEP-518 and PEP-517, so I'll partner on with this. Can you fixup the merge conflicts? And let's see how we can do this.

@codecov
Copy link

codecov bot commented Dec 7, 2017

Codecov Report

Merging #577 into master will increase coverage by 2.37%.
The diff coverage is 74.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #577      +/-   ##
==========================================
+ Coverage    89.5%   91.88%   +2.37%     
==========================================
  Files          11       11              
  Lines        2383     2441      +58     
  Branches      405      419      +14     
==========================================
+ Hits         2133     2243     +110     
- Misses        175      198      +23     
+ Partials       75        0      -75
Impacted Files Coverage Δ
tox/session.py 91.42% <64%> (-0.22%) ⬇️
tox/venv.py 95.45% <92.3%> (+2.93%) ⬆️
tox/config.py 97.42% <92.85%> (+2.52%) ⬆️
tox/__main__.py 0% <0%> (ø) ⬆️
tox/interpreters.py 61.71% <0%> (+2.34%) ⬆️
tox/_pytestplugin.py 93.92% <0%> (+4.04%) ⬆️
tox/_quickstart.py 82.75% <0%> (+5.13%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bbcabd...5ccad40. Read the comment docs.

@gaborbernat
Copy link
Member

gaborbernat commented Dec 15, 2017

I've been pondering over this the last few weeks, and the more I think about this I think conceptually neither PEP-517 and PEP-518 are tox responsibility.

They fall into the pip and setuptools. tox is about creating the virtual environments and running the commands. How the package is built is handled by setuptools, how the packages are installed is handled by pip. So instead adding it to our project I think we should focus on adding these to those projects.

@obestwalter what do you think?

@obestwalter
Copy link
Member

Hi @gaborbernat, might be the case, but I would have to look at what this PR is actually trying to accomplish in detail. I would not merge this into core though even when it makes sense. I would rather work towards making it possible to have this as a plugin. core is already messy enough.

How the package is built is handled by setuptools, how the packages are installed is handled by pip. So instead adding it to our project I think we should focus on adding these to those projects.

To that I agree - or to say it more generalized: How the package is built is handled by the build tool (e.g. setuptools) how the packages are installed is handled by the package manager (e.g. pip or conda). So instead adding it to our project I think we should focus on adding these to those projects.

@aragilar
Copy link

The main problem I run into when setting up tox is dealing with non-setuptools build systems (primarily numpy.distutils, but widespread use of flit is also blocked by this). Pip (or conda) doesn't solve this either, as it's always the sdist component that is problematic (as sdist->wheel/install can be worked around by adding build deps to envs), not the install part. There's a bunch of related issues, which are solved by supporting PEP-518 (e.g. #573, #507, #350, #232). As long as there's no hooks or options to control the sdist process, tox blocks progress in making python packaging better. I'm happy to help with adding hooks and other improvements, but as an outsider I can't see any kind of plan to fix this, and the suggestion to implement this as a plugin seems to gloss over how much this is going to affect (e.g. what happens when use_develop specified)?

@gaborbernat
Copy link
Member

@aragilar you are correct; and thinking this through here's my current plan:

  • PEP-518 is for build dependencies
  • for build system choice the solution is PEP-517 (flit, numpy.distutils) etc.

From tox side now we use ``python setup.py sdist``` to build and then install that with pip into the envs. Instead what we would need to do, first create a venv for building, do PEP-518 file support, install build dependencies; then use the API defined in PEP-517 to create the package, and then feed that to pip, as we do beforehand.

A small side note, build requires is the build tool itself (e.g. flint, setuptools). The build systems may then install further dependencies to build, these are the packages of setup requires (e.g. setuptools_scm). I'm going to create a POC for these the next week inside tox.

@gaborbernat
Copy link
Member

gaborbernat commented Dec 22, 2017

For anyone wondering on the status of this, as a POC (either a playground to test out the rough edges of what this would mean for tox, or as a separate project) I've started doing tox3. It contains a lot of divergences from this (e.g. venv support - fallback to virtualenv, pyproject.toml configuration instead of tox.ini, PEP 517/518 support, asyncio driver - yeah Python 3.6 only runner, but can create python 2 envs) but will come back once I have something usable. WIP - https://github.com/gaborbernat/toxn.

@gaborbernat
Copy link
Member

Drawing from the experience implementing this in toxn (and with pip 10 coming out with PEP-518 support too), I'll add support for this in the next month via another PR. Given the merge conflicts I think it's easier to start clean. For people interested can follow via #573.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature:new something does not exist yet, but should needs:work for PRs: not quite there and needs some changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants