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

Ensure setuptools version when running setup.py. #4753

Merged
merged 3 commits into from Jul 18, 2017

Conversation

Projects
None yet
2 participants
@jsirois
Copy link
Member

jsirois commented Jul 16, 2017

Previously, even though python Interpreter instances were properly
setup with the required setuptools (and wheel) requirement extra,
this was not used to execute setup.py with a custom list of --run
commands.

Fixes #4751
Fixes #4752

Ensure setuptools version when running setup.py.
Previously, even though python `Interpreter` instances were properly
setup with the required `setuptools` (and `wheel`) requirement extra,
this was not used to execute `setup.py` with a custom list of `--run`
commands.

Fixes #4752
@@ -456,9 +469,6 @@ def write_target_source(target, src):
chroot.copy(os.path.join(target.target_base, src, '__init__.py'),
os.path.join(self.SOURCE_ROOT, src, '__init__.py'))

def write_codegen_source(relpath, abspath):

This comment has been minimized.

@jsirois

jsirois Jul 16, 2017

Member

NB: I (IntelliJ) noticed this was unused, so killed while in the neighborhood.

This comment has been minimized.

@benjyw

benjyw Jul 17, 2017

Contributor

Looks like a vestige of the old pipeline. Thanks for cleaning it up.

def mixins(self):
mixins = super(SetupPyRunner, self).mixins().copy()
for (key, version) in self._interpreter.extras:
if key == 'setuptools':

This comment has been minimized.

@jsirois

jsirois Jul 16, 2017

Member

Perhaps this should just loop over all extras and require them as mixins, asserting setuptools since this installer is - in particular - setup.py focused. If so, a follow-up change could fold this up into InstallerBase in pex, which has a handle on the interpreter.

@jsirois jsirois requested a review from benjyw Jul 16, 2017

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 16, 2017

Noting that a test for this is non-trivial, but that's a bit of a cop-out. I think I could concoct a setup.py with a custom distutils entrypoint (exposing a custom setup.py subcommand), and --run that in the test, collecting output and verifying via that output that a custom setuptools was 1st in sys.path during the custom setup.py subcommand run.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Jul 17, 2017

Cop-out overcome. Ready for review.

@benjyw

benjyw approved these changes Jul 17, 2017

Copy link
Contributor

benjyw left a comment

One naming nit. Thanks for fixing!

@@ -456,9 +469,6 @@ def write_target_source(target, src):
chroot.copy(os.path.join(target.target_base, src, '__init__.py'),
os.path.join(self.SOURCE_ROOT, src, '__init__.py'))

def write_codegen_source(relpath, abspath):

This comment has been minimized.

@benjyw

benjyw Jul 17, 2017

Contributor

Looks like a vestige of the old pipeline. Thanks for cleaning it up.

from pants.util.dirutil import safe_mkdir
from pants_test.backend.python.tasks.python_task_test_base import PythonTaskTestBase
from pants_test.subsystem.subsystem_util import init_subsystem


class TestSetupPy(PythonTaskTestBase):
class BaeSetupPyTest(PythonTaskTestBase):

This comment has been minimized.

@benjyw

benjyw Jul 17, 2017

Contributor

A) Bae? Are we mining Urban Dictionary for names now? ;-)

B) Also, this should probably be SetupPyTestBae, er, Base, for uniformity (we generally assume that anything ending in Test tests the thing with the prefix as a name).

This comment has been minimized.

@jsirois

jsirois Jul 18, 2017

Member

Heh, fixed.

@jsirois jsirois merged commit 4f7c91c into pantsbuild:master Jul 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jsirois jsirois deleted the jsirois:jsirois/issues/4752 branch Jul 18, 2017

stuhood added a commit that referenced this pull request Nov 4, 2017

Ensure setuptools version when running setup.py. (#4753)
Previously, even though python `Interpreter` instances were properly
setup with the required `setuptools` (and `wheel`) requirement extra,
this was not used to execute `setup.py` with a custom list of `--run`
commands.

Fixes #4752
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment