Opened 4 years ago

Closed 3 years ago

#7985 enhancement closed fixed (fixed)

Twisted should use sdist to build tarballs instead

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: release management Keywords:
Cc: radix Branch: branches/sdist-use-7985-4
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl

Description


Change History (17)

comment:1 Changed 4 years ago by DefaultCC Plugin

Cc: radix added

comment:2 Changed 4 years ago by hawkowl

Author: hawkowl
Branch: branches/sdist-use-7985

(In [45390]) Branching to sdist-use-7985.

comment:3 Changed 4 years ago by hawkowl

Branch: branches/sdist-use-7985branches/sdist-use-7985-2

(In [45774]) Branching to sdist-use-7985-2.

comment:4 Changed 4 years ago by hawkowl

Keywords: review added

This makes the relevant changes.

To test this, you will need to make an sdist (python setup.py sdist) and compare the files in it to a released tarball. Extract the released one to released, the sdist to sdist. I then compared the files with cd released && find . > ../released.txt && cd ../sdist && find . > ../sdist.txt && cd .. && comm released.txt sdist.txt and inspecting it.

Please review.

comment:5 Changed 4 years ago by Glyph

What is the purpose of this change? Is sdist a moral good in and of itself?

comment:6 Changed 4 years ago by hawkowl

sdist is the moraliest good.

But, in seriousness, this gives us:

  • Standardisation with other Python projects, in the realm of packaging. Making a tarball is now the same as every other project.
  • sdist does some metadata creation that lets our tarballs be uploaded by twine, so it means we don't have to reinvent the PKG-INFO creator (which I don't even know where we'd start), and we can automate our releases a little bit more.
  • We remove a bunch of stuff that existed because sdist wasn't good, I'm assuming, and now it's better. The code does not need to exist now.

-hawkie

comment:7 Changed 4 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl
recursive-include twisted *.misc *.bugfix *.doc *.feature *.removal

do we need these files in the release? I was expecting that the release will remove all these files and we will not release code with those files.

Why are those files Topfiles ?


Do we need to distribute tox.ini and _preamble.py with the sdist archive?

---

Is this line ok ? It looks like mixin module names with path names.

args['py_modules'] = modulesToInstall + testDataFiles

or we should rename testDataFiles to testDataModules

It has no news file fragment.


This branch removes a couple of tests but no new tests are added. Does the existing tests provide covergate for the setup.py sdist changes?

Does this branch comes with the associated documentation which instruc what should we use not for release instead of the removed code?

Please check my comments and resubmit.

I am happy to see that we can use the setup.py sdist but I think that it needs at least a few changes in the documentation.

Thanks!

comment:8 Changed 4 years ago by hawkowl

https://twistedmatrix.com/trac/ticket/7759 would be fixed by this ticket.

comment:9 Changed 3 years ago by hawkowl

Branch: branches/sdist-use-7985-2branches/sdist-use-7985-3

(In [46888]) Branching to sdist-use-7985-3.

comment:10 Changed 3 years ago by hawkowl

Keywords: review added
Owner: hawkowl deleted

Builds look good, I'm happy. Please review.

comment:12 Changed 3 years ago by Adi Roiban

Keywords: review removed

Major comments:

  1. I assume the plan is to run the tests suite from the sdist and in this way also test the whole sdist thing. This is not documented. Maybe MANIFEST.in should start with a comment documenting what files should be included here and based on which rules.
  1. The replacement for bin/admin/build-tarballs is not documented ... and the removed command is still documented at https://twistedmatrix.com/trac/wiki/ReleaseProcess
  1. Please include a news fragment. I would say that it should have a .feature one advertising package developers that the distribution script were change.

Minor comments

  1. Instead of 'PickyBuildPy' maybe we can have 'BuildOnlyPortedModules'. Same for PickyBuildScripts... which would be nice to have a docstring describing its purpose.
  1. I would say something similar to DistributionBuilderTests.test_twistedDistribution should have been part of the new patch... but I hope that by integrating sdist into our continuous tests we can cover these changes in this way.

Looking forward for having tox as our buildslave driver and put the changes from tox.ini under continuous testing.

Please check my comments and merge.

Thanks!

comment:13 Changed 3 years ago by hawkowl

Hi Adi, thanks for the review.

I've re-opened, done the work, and put up https://twistedmatrix.com/trac/ticket/4543 for review, which will add the release process to version control. Once #4543 is merged, I will update this ticket with the updated Twisted release process, put it up for review, and then both the process and the implementation can be merged without the other being out of date.

comment:14 Changed 3 years ago by hawkowl

Branch: branches/sdist-use-7985-3branches/sdist-use-7985-4

(In [46932]) Branching to sdist-use-7985-4.

comment:15 Changed 3 years ago by hawkowl

Keywords: review added

I've added:

  • Comments about the MANIFEST.in
  • Documented the replacement of build-tarballs in the release process
  • Added a news fragment

As per testing the sdist elements, we have a check-manifest builder which will go green after this and perform the same duty as the old tests did, and (very) soon we will move to installed testing in tox, where the setup will be put under continuous test.

Builders spun. Please review.

comment:16 Changed 3 years ago by Adi Roiban

Keywords: review removed
Owner: set to hawkowl

Looks good. Thanks!

I guess that to really be able to review/test all this, together with the documentation it would require to have a different release manager for on release.

I trust you that the information is accurate :) so please merge as we don't need to make it more complicated :)

Thanks!

comment:17 Changed 3 years ago by hawkowl

Resolution: fixed
Status: newclosed

(In [46942]) Merge sdist-use-7985-4: Use setup.py sdist to build tarballs

Author: hawkowl Reviewer: adiroiban Fixes: #7985

Note: See TracTickets for help on using tickets.