Opened 2 years ago

Closed 2 years ago

#8622 enhancement closed fixed (fixed)

Remove logic from setup.py

Reported by: mark williams Owned by: Adi Roiban
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch:
Author:

Description

As [8606#3 discussed here], there's non-trivial code that's in setup.py (and at this time, setup3.py, though that will change when #8606 is closed)

It's difficult to test any logic in setup.py. Instead, setup.py should be made as simple as possible, and as much code as possible should be moved into twisted.python.dist where it can be tested.

adiroiban has done significant work in this direction.

Change History (11)

comment:1 Changed 2 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted

PR at https://github.com/twisted/twisted/pull/467

All previous methods which were just returning a static data structure based on python version were removed

setup.py is now just calling twisted.python.dist.setup() and that is all

In previous version we had both data structured and build logic split between setup.py and dist.py. Now everything was moved to dist.py

As a drive-by I have removed admin/run-python3-tests and admin/run-python3-tests-venv as I don't think they we are using them.


twisted.python.dist3 module was renamed as twisted.python._python3_port as I don't see why we need to have it as a public API

Also, I don't see why twisted.python.dist is a public API. Does external projects need to know about how Twisted is defining its setup() method?

twisted.python.dist.getVersion and build_scripts_twisted were removed without deprecation as they are no longer used in the Twisted code base.

I did not went for the deprecation dance, since the twisted.python.dist docstring starts with Don't use this outside of Twisted. ... so this is like a private API.

I will follow up over the mailing list to let other know about these changes.

Meanwhile please check that the general direction of this ticket is OK.

Thanks!

comment:2 Changed 2 years ago by Adi Roiban

The updated tests are tricky since then executed inside or outside tox we will have a different working directory... there is a different path for location where the twisted source code is... so that we can run some setup.py tests

I am testing this as:

$ ./bin/trial twisted/python/test/test_setup.py
$ tox -e py27-coverage-posix twisted.python.test.test_setup

I went for introducing a TOX_INI_DIR env var and I don't have a better idea, I find if better than start mocking/patching setuptools/distutils

comment:3 Changed 2 years ago by hawkowl

Keywords: review removed
Owner: set to Adi Roiban

Hi Adi, please address the comments on the PR, then land it.

comment:4 Changed 2 years ago by Adi Roiban <adi.roiban@…>

Resolution: fixed
Status: newclosed

In 41482c4:

Merge 8622-setup-coverage: Move all setup.py related code to t.python._setup.

Author: adiroiban
Reviewer: hawkowl
Fixes: #8622

comment:5 Changed 2 years ago by Adi Roiban

Many thanks for the review!

comment:6 Changed 2 years ago by Adi Roiban <adi.roiban@…>

In f96bc30f:

Revert "Merge 8622-setup-coverage: Move all setup.py related code to t.python._setup."

Reopens: #8622

'twist' script was left out when resolving conflicts on an update with trunk.

comment:7 Changed 2 years ago by Adi Roiban

Resolution: fixed
Status: closedreopened

The new "twist" script was left out during a merge conflict resolution.

comment:8 Changed 2 years ago by Adi Roiban

Keywords: review added
Owner: Adi Roiban deleted
Status: reopenednew

I am putting this back for review without adding tests for console scripts.

I think there is no point to adding unit tests in t.p.test.test_setup as it will just check the declaration of a list.

I think that console scripts can be unit tested using something like this

    from pkg_resources import load_entry_point
    args = ['something']
    main = load_entry_point(
        'twisted', 'console_scripts', 'twist')

but then we need to make sure that all tests environments are running pip install before running the tests.

There is ongoing work in removing bin/trial and having all buildbot builders executed with tox ... but we are not there yet.

So, after all the dev/test environment have pip install -e . or something similar, we can look at adding tests for the scripts.

comment:9 Changed 2 years ago by Craig Rodrigues

Keywords: review removed
Owner: set to Adi Roiban

comment:11 Changed 2 years ago by Adi Roiban <adi.roiban@…>

Resolution: fixed
Status: newclosed

In 24d4b0d1:

Merge 8622-setup-coverage: Re-merge refactoring of setup.py

Author: adiroiban
Reviewer: hawkowl, rodrigc
Fixes: #8622

Note: See TracTickets for help on using tickets.