-
-
Notifications
You must be signed in to change notification settings - Fork 531
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
Expose the packaging as a plugin hook #952
Conversation
path = self.get_installpkg_path() | ||
if not path: | ||
return 2 | ||
for venv in self.venvlist: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now allow separate packages per virtual environment. The use case for this extension is to allow e.g. binary packages (might need different compilation) and/or wheels (different wheel per venv).
Codecov Report
@@ Coverage Diff @@
## master #952 +/- ##
==========================================
- Coverage 92.85% 92.77% -0.09%
==========================================
Files 12 13 +1
Lines 2379 2393 +14
Branches 413 416 +3
==========================================
+ Hits 2209 2220 +11
- Misses 107 109 +2
- Partials 63 64 +1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise looks good -- this is mostly just a code reorg right?
try: | ||
path = make_sdist(report, config, session) | ||
except tox.exception.InvocationError: | ||
v = sys.exc_info()[1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I know this is existing, but I couldn't help notice it) -- shouldn' tthis be as v:
and not retrieving sys.exc_info()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it should 👍
if not path: | ||
return 2 | ||
for venv in self.venvlist: | ||
venv.package = self.hook.tox_package(session=self, venv=venv) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ok, this will cause sdist to be called N times (previously 1) -- should we make this use the virtualenv's python as well in this pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be a breaking change. This is mostly just reorg to have a clear separation of packaging and plugin-ablity. So not just yet, maybe with tox 4. For now, the plan is to just keep using the host python to build the venv.
I'll have a follow-up PR that will create a new virtualenv for building the package, parse the pyproject.toml, install those dependencies. That will effectively implement PEP-517/518 as an opt-in 👍 As external plugin people can implement wheel support, etc. I'll retract my #861 as such.
Resolves #951. This will allow having PEP-517/518 for the end users.