Opened 8 years ago

Closed 6 years ago

#7177 enhancement closed fixed (fixed)

Move to requiring setuptools

Reported by: Alex Gaynor Owned by: hawkowl
Priority: normal Milestone: Python-3.x
Component: core Keywords:
Cc: Adi Roiban Branch: branches/setuptools-7177
branch-diff, diff-cov, branch-cov, buildbot
Author: ralphm, hawkowl

Description (last modified by hawkowl)

setuptools is, practically speaking, available on every installation platform you care about, since pip makes it happen.

Change History (15)

comment:1 Changed 8 years ago by Alex Gaynor

Keywords: review added

comment:2 Changed 8 years ago by Ralph Meijer

Keywords: review removed
Owner: set to Alex Gaynor

This mostly looks good. However, the news file doesn't conform to our standard. It is meant to be a high level description of what happened, with the subject being the module that changed and a verb prefixed by now. In this case it would need to be something like:

Twisted now depends on ...

You probably don't want to mention 'setup.py' at all.

comment:3 Changed 8 years ago by Alex Gaynor

Keywords: review added
Owner: changed from Alex Gaynor to Ralph Meijer

The PR has been updated to address this. Thanks!

comment:4 Changed 8 years ago by Ralph Meijer

Author: ralphm
Branch: branches/setuptools-six-7177

(In [42434]) Branching to setuptools-six-7177.

comment:5 Changed 8 years ago by Ralph Meijer

Keywords: review removed
Owner: changed from Ralph Meijer to Alex Gaynor

The branch is fine now, but it turns out that it must block on #3696 (which #7092 was marked a duplicate of), because after/while landing, six would need to be installed on all our build slaves.

comment:6 Changed 8 years ago by radix

I'm not sure why this must block on #3696 -- if we depend on six, we depend on it strictly, not optionally.

I think the only thing blocking it is somehow running on the buildslaves -- whether by updating our build steps to automatically install dependencies, or by manually installing six on all of them.

comment:7 Changed 8 years ago by Ralph Meijer

I think #7092 might have been a better description, but glyph made that a dupe of #3696 when we discussed this problem. Feel free to undupe. Manually installing stuff is a bad idea. The patch itself is clearly making it a hard dependency.

comment:8 Changed 7 years ago by Adi Roiban

Cc: Adi Roiban added

comment:9 Changed 6 years ago by hawkowl

Author: ralphmralphm, hawkowl
Branch: branches/setuptools-six-7177branches/setuptools-7177

(In [45377]) Branching to setuptools-7177.

comment:10 Changed 6 years ago by hawkowl

Keywords: review added
Owner: Alex Gaynor deleted

I don't think we need six until we have something actually using it. Otherwise, this is getting in a shape. Please review.

comment:11 Changed 6 years ago by hawkowl

(builders spun, also)

comment:12 Changed 6 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Please update the title of this ticket. It says 'Add six as a dependency ... ' but no six is added here. The description is fine as it does not talk about six.

as long as we continue to duplicate six code into twisted.python.compat we don't need six... and since we don't depend on six people will continue to duplicate code in compat rather than using six.

I guess that this ticket/branch is only about the migration to setuptools.


I think we should document that Twisted depends on setuptools, at leaset in the INSTALL file.

As far as I know, pip can also work with Distribute.

I see that in 'very old' versions of setuptools there used to be a bug with install_requires.

I am ok with not handling that error in the code but I think that we should document the version required for Twisted.

Please address my comments and merge.

Thanks!

comment:13 Changed 6 years ago by hawkowl

Description: modified (diff)
Summary: Add six as a dependency and move to requiring setuptoolsMove to requiring setuptools

comment:14 Changed 6 years ago by hawkowl

Hi Adi, thanks for the review.

  • Ticket title changed.
  • I couldn't find a version of setuptools that exhibited that bug -- I asked lifeless and he hadn't heard of such a bug existing, so I tried the oldest ones I could on PyPI and they all worked fine.
  • I'll add the requirement for setuptools.

comment:15 Changed 6 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [45435]) Merge setuptools-7177: Require setuptools for installation.

Author: Alex, hawkowl Reviewers: ralphm, adiroiban Fixes: #7177

Note: See TracTickets for help on using tickets.