Ticket #4138 (new enhancement )

Opened 2 months ago

Last modified 3 weeks ago

A fresh Twisted checkout should support "setup.py sdist"

Reported by: TimAllen Assigned to: TimAllen
Type: enhancement Priority: normal
Milestone: Component: core
Keywords: Cc: exarkun, radix, thijs, tarek
Branch: branches/sdist-support-4138 Author: TimAllen
Launchpad Bug:

Description

This ticket is a stepping-stone towards #1696, but this is an independent, self-contained sub-goal that I expect will involve lots of discussion and exploration of alternatives, so I thought I'd file a new ticket.

Steps to reproduce:

  1. Download the official Twisted 9.0.0 source tarball
  2. Run the following commands:
    tar tjf Twisted-9.0.0.tar.bz2 | sort > /tmp/official-tarball-files
    tar xjf Twisted-9.0.0.tar.bz2
    cd Twisted-9.0.0
    ./setup.py sdist
    tar tzf dist/Twisted-9.0.0.tar.gz | sort > /tmp/sdist-tarball-files
    
  3. Compare /tmp/official-tarball-files and /tmp/sdist-tarball-files with your favourite diff tool.

Expected results:

  • File lists should be identical, at least as far as executable code and tests go.

Actual results:

  • The sdist tarball does not include Pyrex source files for Twisted's extension modules, or various test-data files required by test suites, which rather cripples the resulting tarball.
  • The sdist tarball also does not include documentation, but I'm not so fussed about that (see "as far as executable code and tests" above).

Why this is a problem:

  • People (including but not limited to me) want to build RPMs of Twisted so they can depend on newer versions of Twisted than are shipped with their production platform (say, CentOS or RHEL).
  • Building RPMs with bdist_rpm is superior to building RPMs with admin/twisted.spec because:
    • Nearly every Python project that uses distutils can be built as an RPM this way, reducing documentation and training needs.
    • For those whose production systems depend on changes to Twisted that will be pushed upstream, running bdist_rpm from the branch is much, much simpler than writing a build-system to download official tarballs and bundle a patch-set and custom twisted.spec.
  • Working sdist is a precondition for working bdist_rpm.

Attachments

sdist-support-4138.2.patch (44.0 kB) - added by TimAllen 1 month ago.
Addressed (most of) glyph's review comments.

Change History

  2009-11-27 07:38:27+00:00 changed by TimAllen

The traditional solution for messing with the files sdist includes is by using a MANIFEST or MANIFEST.in file, however in #1696 I mentioned an IRC conversation with radix in which he mentioned he would argue against any patch that duplicated the what-files-go-where logic in the existing Twisted release scripts. Thus, I have a proof-of-concept patch for hooking sdist up to the innards of those release scripts.

The changes are all glommed together into one patch, so here's an overview of what's changed:

  • In previous code-reviews, people have pointed out that Twisted's code style requires three blank lines between top-level constructs and two blank lines between methods of a class. Some of the files I've touched didn't conform to this, so I fixed up whatever whitespace problems I found in those files.
  • The global variable twisted_subprojects has moved from t.p.dist (where it wasn't used) to t.p._release where it's used extensively. A later change makes t.p.dist import from t.p._release and moving this variable across prevents a cyclic import dependency.
  • t.p.dist.get_setup_args() now adds its custom cmdclass values to the ones supplied by the setup.py file, so a setup.py can redefine one without having to redefine them all.
  • t.p._release.DistributionBuilder.buildTwisted() previously walked the Twisted directory tree and added things to a tarball. I've broken out the 'walk the directory tree' code into a new method iterateTwistedPaths(), so that other things that want to get the exact list of files to package can do so without having to build a tarball.
  • t.p.dist now defines an sdist_twisted command that builds its list of files by calling iterateTwistedPaths() instead of reading a MANIFEST or MANIFEST.in. The root setup.py uses this implementation instead of the distutils default sdist.

I've included unit-tests for most of the code changes, but there are no tests for t.p.dist.sdist_twisted because it depends on knowing the path to the root of the Twisted checkout, and I'm not sure how to find that reliably. I'd pass it in, except that I can't modify the class interface because it must conform to what distutils expects.

Now that I think about it, perhaps I could include an optional parameter to the constructor that distutils wouldn't call, but which I could call from the test suite...

Anyway, this patch is more to provoke ideas and discussion than seriously for review.

  2009-11-27 07:49:41+00:00 changed by TimAllen

I forgot to mention: After the attached patch is applied, the differences between an official tarball and an sdist tarball are reduced to:

  • sdist tarball includes entries for directories, not just files
  • sdist tarball includes documentation with a .xhtml extension, not .html
  • sdist tarball includes man pages in troff but not in HTML.
  • sdist tarball includes some .pyc files (not a problem for release tarballs because the checkout that's packaged is not the checkout where the release-script is run).

  2009-11-27 19:11:33+00:00 changed by glyph

Thanks for this wonderfully detailed description.

  2009-11-27 22:53:13+00:00 changed by exarkun

  • cc set to exarkun

What if setup.py sdist just invoked (probably some sub-set of) the code which is currently invoked by bin/admin/build-tarballs? In particular, it seems like calling DistributionBuilder.buildTwisted is almost exactly the right thing to do.

It seems like having two ways to build a source distribution tarball which produce different results is a good way to get into trouble.

Is the tarball produced by buildTwisted not suitable as a result of setup.py sdist?

  2009-11-30 06:52:47+00:00 changed by TimAllen

If you'd asked me that a week ago, I might have thought it impossible. Having learned a bunch about distutils innards in the intervening time, I thought I'd have a go.

There's one major difference between the base assumptions of buildTwisted and the base assumptions of distutils' sdist: buildTwisted assumes it will be packaging a pristine, temporary SVN checkout, and so it defaults to 'include everything apart from this blacklist'. sdist assumes it will be packaging the package it's running from, and so it defaults to 'exclude everything apart from this whitelist'.

However, I've managed to bodge together an even hackier (i.e. no tests) proof-of-concept that includes properly-built documentation and manpages. It doesn't just call buildTwisted directly, because that would neuter sdist's rather useful --formats option.

The changes between an official 9.0.0 tarball and the one produced by sdist with this new patch include:

  • Official tarballs are always .tar.bz2, sdist defaults to .tar.gz but can be configured to build any combination of .tar, .tar.Z, .tar.gz, .tar.bz2, .zip.
  • sdist includes a Twisted-9.0.0/PKG-INFO file, which contains the setup.py metadata in some RFC822-based format. It's explicitly included by the standard sdist implementation, so I felt I should include it too.
  • sdist has dropped the executable bit from various files; if there's a better tool than t.p.filepath.FilePath.copyTo() I'd like to hear about it.
  • sdist includes 52 .pyc files and any Vim swap files, etc. that are lying around.
  • sdist includes entries for Twisted-9.0.0/, Twisted-9.0.0/bin/, Twisted-9.0.0/doc/, Twisted-9.0.0/twisted/, Twisted-9.0.0/twisted/plugins/.
  • The official tarballs include Twisted-9.0.0/twisted/trial/test/directory/, which is detritus from the test suite (see #3101).
  • There are strange differences in the generated documentation, but they don't seem to be significant. For example:
    --- a/tmp/official-twisted/Twisted-9.0.0/doc/core/development/policy/coding-standard.html
    +++ b/tmp/patched-sdist/Twisted-9.0.0/doc/core/development/policy/coding-standard.html
    @@ -114,9 +114,9 @@ $ bin/trial twisted/mail/pop3.py
     <pre class="python"><p class="py-linenumber">1
     2
    -</p><span class="py-src-comment"># Copyright (c) 2009 Twisted Matrix Laboratories.</span>
    -<span class="py-src-comment"># See LICENSE for details.</span>
    -</pre>
    +</p><span class="py-src-comment"># Copyright (c) 2009 Twisted Matrix Laboratories.
    +</span><span class="py-src-comment"># See LICENSE for details.
    +</span></pre>
         <p>When you update existing files, make sure the year in the copyright
         header is up to date as well. You should add a new copyright header when
    

This is slightly less efficient than both the existing sdist and existing Twisted distribution builders:

  • Unlike sdist, this code never takes advantage of hard-links to speed up building the directory tree to be archived.
  • Unlike the previous incarnation of buildTwisted, this code does not create a tarball directly, it just creates a full directory tree with the desired contents and archives it.

  2009-12-01 05:49:33+00:00 changed by TimAllen

  • cc changed from exarkun to exarkun, radix
  • keywords set to review
  • owner deleted

I still don't expect this to be ready for merging, but I would like feedback on whether there might be better approaches for integrating with Twisted's distribution-building code, for copying files and preserving permissions, and why the Lore docs are reformatted.

Also, Cc'ing radix because this patch messes rather intimately with his release-making code.

  2009-12-09 22:51:15+00:00 changed by exarkun

  • keywords deleted
  • owner set to TimAllen

As you say, it's not done, but I do like the direction the new patch is taking. We should be careful around the overridden sdist methods, be sure to test that functionality well, document whatever assumptions we're making, etc, and maybe get Tarek Ziade's opinion on it, but I think it could work.

As far as the Lore reformatting goes, what were you comparing? Lore uses xml.dom.minidom now; could differences in the version of that used be introducing these differences?

Thanks for your work on this!

  2009-12-10 06:37:22+00:00 changed by TimAllen

  • status changed from new to assigned

I've spent an hour or two tinkering with code trying to make the test suite set the executable bit on files in its test directory structures, and then check the executable bit packaging, but it's pretty maddening; I fear I shall have to add a "copyStat" parameter to FilePath.copyTo() that makes it call shutil.copystat() on each file it copies.

As for the Lore changes, my code is directly based on the 9.0.0 release; I assume radix built the 9.0.0 tarball using the version of Lore from the 9.0.0 release, as did I - but I haven't changed anything Lore related (as far as I know) and I get the subtly different output I pasted above. On the other hand, I've been doing my testing with Python 2.4, and I guess radix is using 2.5 or 2.6 - if minidom has changed in some way, I guess that might explain it.

  2009-12-10 14:02:04+00:00 changed by exarkun

I fear I shall have to add a "copyStat" parameter to FilePath?.copyTo() that makes it call shutil.copystat() on each file it copies.

Also, don't be afraid to add a private helper to dist.py or _release.py or wherever that does exactly what you want without any concerns for generality.

On the other hand, I've been doing my testing with Python 2.4, and I guess radix is using 2.5 or 2.6 - if minidom has changed in some way, I guess that might explain it.

Yea. What if you generate the 9.0.0 docs yourself w/o your changes and compare that to your generated docs w/ your changes (using the same version of Python each time, of course)?

  2009-12-11 08:20:05+00:00 changed by TimAllen

Another checkpoint - the newly attached patch adds t.p._release._stageFile that largely replicates what distutils code did - it hard-links files (if it can) or copies them with metadata (otherwise). This means all the files that should be executable in the resulting tarball, are.

Things I still intend to do before review:

  • filtering out .pyc and perhaps .pyo files. Black-listing particular file extensions is a slippery slope (how many editor-backup-file extensions are there?), but I've already discovered (from #1696) that Twisted has too many random files to make white-listing feasible.
  • Try and build an official Twisted tarball on my workstation and compare the Lore output.

Things I will do if asked, or for bonus points:

  • Track down Tarek Ziade, try and explain what my code does, see if he has an opinion.
  • Port the other distribution-building methods to use the same structure as buildTwisted - no performance or compatibility benefit, just consistency within the release code.

About generating the 9.0.0 docs myself on the trunk: it seems the bin/admin/build-tarballs script requires an SVN checkout, and I've been using git-svn all this time. I'll try an SVN checkout and try to get it working later, but not today.

  2009-12-11 10:37:13+00:00 changed by thijs

  • keywords set to review
  • owner deleted
  • status changed from assigned to new

Putting it in the review queue.

  2009-12-14 06:04:16+00:00 changed by TimAllen

I used SVN to check out today's trunk, and ran the bin/admin/build-tarballs script to produce Twisted-9.0.0.tar.bz2. Then, I rebased my changes onto the trunk and used sdist to produce Twisted-9.0.0.tar.gz. I untarred them both side-by-side, removed the .pyc files from the sdist tree, and compared the two.

Results: The only differences were the changes in my patch. The Lore differences I observed must be a Python/minidom thing, as exarkun surmised.

  2009-12-14 07:19:39+00:00 changed by TimAllen

OK, the latest attachment adds a quick hack (+ tests, naturally) to make _stageFile() quietly ignore any attempts to stage .pyc or .pyo files. With that, the only differences between the sdist tarball and one produced by the official Twisted script are:

  • sdist packages include a PKG-INFO file in the root directory. Apparently this is a distutils thing, and I am loathe to change it.
  • Official tarballs include twisted/trial/test/directory/ due to #3101.
  • sdist packages include entries for various directories, which shouldn't affect anything.
  • sdist packages default to .tar.gz although you can build .tar.bz2 as well.
  • sdist packages built with this patch *include* this patch, tarballs built from the trunk do not.

And with that, I believe this patch is ready for the usual in-depth review. Lay on, MacDuff! ;)

  2009-12-24 17:14:15+00:00 changed by radix

  • keywords deleted
  • owner set to Screwtape

Wow, I'm happy that you were able to do this without doing anything with private distutils APIs. Unless I missed something...

1.

+        _, extension = src.splitext()

I prefer "extension = src.splitext()[1]". In fact, since the variable is only used once, you could just in-line the expression in the conditional on the following line.

2. Can you compare to errno.EXDEV instead of the literal number 18?

3. I think it's worth being explicit in _stageFile's docstring that hard links will be made if possible.

4.

-        if sys.version_info[:3] < (2, 3, 0):
-            kw['cmdclass']['build_py'] = build_py_twisted
+    if sys.version_info[:3] < (2, 3, 0):
+        defaultCmdClasses['build_py'] = build_py_twisted

This code can probably be deleted, since we don't even support Python 2.3 any more, let alone anything less than that.

5. When I run "python setup.py sdist" from an SVN checkout, the resulting tarball contains .svn directories. These should be filtered out.

  2010-01-04 03:30:52+00:00 changed by TimAllen

  • keywords set to review
  • owner changed from Screwtape to TimAllen

I don't think I've used any private distutils stuff; I've messed with undocumented things, but then almost everything in distutils is undocumented.

I've attached a new version of the patch with the following changes:

  1. Added a module constant to t.p._release.py named EXTENSION_BLACKLIST, containing the extensions of bytecode files and of various VCS tools that people might use with Twisted.
  2. Updated _stageFile to ignore files or directories with extensions in EXTENSION_BLACKLIST.
  3. Updated _stageFile's docstring, as requested.
  4. Merged the splitext() line into the if statement, as requested.
  5. I have now discovered the errno module in the standard library, thank you. :)
  6. I've removed the build_py_twisted class from t.p.dist and the various places it was used.

Back to review!

  2010-01-04 17:55:56+00:00 changed by thijs

  • cc changed from exarkun, radix to exarkun, radix, thijs

Is #3912 a duplicate of this one?

  2010-01-04 19:08:16+00:00 changed by exarkun

Hard to say yet. However, even if it turns out to be, the main blocking issue for #3912 is test coverage, which probably involves some buildbot configuration. So we shouldn't close #3912 until it's CIS-tested somehow.

  2010-01-05 01:12:27+00:00 changed by TimAllen

I would say that #3912 is not a duplicate of this ticket, but merging this branch would partially fix #3912 by happy accident. A proper fix for #3912 would probably include doing the same fix to all the subproject setup.py files too.

  2010-01-05 01:12:59+00:00 changed by TimAllen

  • owner deleted

  2010-01-05 01:16:18+00:00 changed by thijs

Could you cover a fix for #3912 in this ticket so we can use that other ticket for the subproject's? Because you say #3912 would be partially fixed by this ticket already.. So a unit test for the main setup.py version nr issue here would be nice because doesn't tap2rpm kinda depend on a consistent version nr anyway? Or is this ticket even blocked by #3912..

  2010-01-05 01:21:42+00:00 changed by thijs

  • cc changed from exarkun, radix, thijs to exarkun, radix, thijs, tarek

  2010-01-05 03:04:24+00:00 changed by TimAllen

I'd rather not absorb #3912 here, honestly. I don't know that this ticket would fix #3912, I just know that it does the same kind of "override sdist" trick that the patch in #3912 does. I took a look at setuptools' patched sdist and I can't see any evidence that it messes with the version string at all, so I don't understand how the patch in #3912 could possibly work.

Digging up enough information on setuptools to find out what *would* work is a bit far removed from what the rest of the work in this branch, and I think really belongs as a separate ticket.

  2010-01-05 03:18:26+00:00 changed by exarkun

I'd rather not absorb #3912 here, honestly.

That's sufficient reason for me. :)

follow-up: ↓ 25   2010-01-07 09:54:39+00:00 changed by glyph

  • keywords deleted
  • owner set to TimAllen

Thanks again for plugging away on these gnarly edge-cases!

  1. I don't really like how EXTENSION_BLACKLIST works. For example, why .git and .bzr, but not .hg? Why not .DS_Store? Why not .nautilus_preview_media_turd or whatever that's going to be called in the next version? For one thing, I think that all hidden files should be ignored, but more importantly I think that the entry point to the should-this-file-be-included logic should be in a fucnction. "if src.splitext()[1] in EXTENSION_BLACKLIST" is repeated twice, and that's just on the border of an acceptable amount and complexity of code to repeat :).
  2. The blacklisted-ness should be checked before trying to stage the file. If, for example, you have an Emacs buffer open, that will create a dead symbolic link as a lock file. Those dead symbolic links are hidden, but if you check to see if they're files or directories first, you'll blow up with "can only stage files or directories" despite the fact that it should have been ignored in the first place.
  3. sdist_twisted's name violates the coding standard, I think. Does it need to have an underscore in it for some reason? If not, SDistTwisted would be the right thing to call it. The other stuff in that file also seems to be violating the coding standard but I can't see that it needs to; new stuff should be done right.
  4. Should sdist_twisted actually be public? It seems like it might be generally useful (for the Divmod projects, at least) but maybe it would be better if it began its life in _release? I could be wrong about this: I'm not really clear on the intended division of labor in these modules. But I'd err on the side of making it private.
  5. It also needs a better docstring. What is 'our way'?
  6. get_file_list and make_release_tree look like they're overriding functions from sdist.sdist, but you should probably mention that in order to avoid getting flack from another review about coding-standard compliance on their names :).
  7. We're trying to promote a more assertive style in test docstrings. For example, instead of "_stageFile should duplicate the content and metadata of a single file", "_stageFile duplicates the content and metadata of a single file." (Also: which file?)
  8. Wow. Hard links, really? This kind of stuff should be in FilePath. Especially since you're duplicating the EXDEV-handling logic that moveTo has, and you're accessing '.path' a lot. I think I'd start off with a simple .copyTo, and have a separate ticket for optimizing it with hard links when that's possible. Moreover: your EXDEV handling codepath is untested, which makes this even dodgier.
  9. In test_release, "if "/bin" in child.path" almost certainly wants to be "if "bin" in child.segmentsFrom(root):". I know these tests are UNIX-only so we don't need to worry about the literal slash, but if your Twisted checkout happens to be in a directory called "/media/bingo" you are likely to see some bad results from that test. Moral of the story: use FilePath, it will save you from yourself.
  10. makeAPIBaseURL needs a @param and a @return and maybe some @rtypes. It's especially important to be clear about that stuff when you're passing around strings (which could be text or bytes) that represent something else (and could therefore be URLPaths or FilePaths or whatever).

Otherwise I think it's looking pretty good.

in reply to: ↑ 24   2010-01-11 03:24:16+00:00 changed by TimAllen

Replying to glyph:

10. makeAPIBaseURL needs a @param and a @return and maybe some @rtypes.

Fixed.

3. sdist_twisted's name violates the coding standard, I think.

Yeah, I copied the name from the surrounding examples; I've changed it to SDistTwisted.

4. Should sdist_twisted actually be public?

I can see an argument for SDistTwisted being private, since it's not really useful for any other project (or anything outside Twisted's SVN tree), but that argument would probably apply to a bunch of other things in t.p.dist like get_setup_args(). If there is a division between dist and _release, I'd say it's that dist contains modifications to distutils, and _release contains Twisted's home-grown release system, so SDistTwisted should stay in dist. I wouldn't regard either of them as public modules, though.

5. It also needs a better docstring. What is 'our way'?

6. get_file_list and make_release_tree look like they're overriding functions from sdist.sdist

7. We're trying to promote a more assertive style in test docstrings.

I've updated my various test docstrings accordingly.

1. For one thing, I think that all hidden files should be ignored, but more importantly I think that the entry point to the should-this-file-be-included logic should be in a function.

I have just discovered an almost identical system in t.p.dist in the EXCLUDE_NAMES and EXCLUDE_PATTERNS globals, which exclude .svn and CVS and {arch} and _darcs but not .git or .bzr or .hg. I think some centralisation is in order.

I've removed t.p.dist._filterNames and its associated blacklists and combined them with the EXTENSION_BLACKLIST stuff into a new method called t.p._release.isDistributable(), which is a lot simpler. So far as I can tell, the only user of _filterNames was t.p.dist.getDataFiles(), which now has a fairly comprehensive test suite and has been rewritten to use isDistributable().

The related function t.p.dist.relativeTo() is a shambling horror that is required to make getDataFiles() do its thing. We should probably get radix to check whether relativeTo()'s... eccentric behaviour is actually required, and remove it if we can. At the very least, I've updated the docstring to show what it *does* do, rather the tragically misleading content it had before.

2. The blacklisted-ness should be checked before trying to stage the file.

Fixed.

8. Wow. Hard links, really? This kind of stuff should be in FilePath. Especially since you're duplicating the EXDEV-handling logic that moveTo has, and you're accessing '.path' a lot. I think I'd start off with a simple .copyTo, and have a separate ticket for optimizing it with hard links when that's possible.

Although it was a while ago now, I seem to recall a conversation with exarkun on #twisted where he recommended writing a custom, specific function rather than trying to fit something into FilePath. In particular, a .copyTo method could not be optimized to use hard-links (because copies should be independant) nor could a hypothetical .linkTo method fall back to copying (links should be... linked). Therefore, I'd need a custom function for my quirky use-case. Since I need a custom function anyway, I might as well make my custom function do the whole operation (copying content, copying metadata, hard-linking where possible).

Moreover: your EXDEV handling codepath is untested, which makes this even dodgier.

I've added a test for EXDEV handling.

Also, during testing I noticed that trying to stage one file on top of an existing file raised different errors depending on whether it used hard-links or copying, so I've taught _stageFile to paper over that difference.

9. In test_release, "if "/bin" in child.path" almost certainly wants to be "if "bin" in child.segmentsFrom(root):". I know these tests are UNIX-only so we don't need to worry about the literal slash, but if your Twisted checkout happens to be in a directory called "/media/bingo" you are likely to see some bad results from that test. Moral of the story: use FilePath, it will save you from yourself.

You're right; if I hadn't had FilePath I probably should have split on os.sep or something. It turns out that because createStructure() is recursive, "child.segmentsFrom(root)" didn't do exactly what I wanted, but I changed things about a bit to make it work.

  2010-01-11 03:36:59+00:00 changed by TimAllen

  • attachment sdist-support-4138.2.patch added

Addressed (most of) glyph's review comments.

  2010-01-11 03:38:26+00:00 changed by TimAllen

  • keywords set to review
  • owner deleted

  2010-01-11 17:40:14+00:00 changed by exarkun

  • branch set to branches/sdist-support-4138
  • branch_author set to exarkun

(In [27953]) Branching to 'sdist-support-4138'

  2010-01-11 19:00:57+00:00 changed by exarkun

  • keywords deleted
  • owner set to TimAllen
  • branch_author changed from exarkun to TimAllen

So, there's one big issue that needs to be resolved here. It's about which way the various release-related module dependencies go, and what platforms they're all supported on. Currently, in trunk, twisted.python._release claims to only work on Linux. Its test suite only runs on Linux (and if it weren't skipped on other platforms, some of its tests would fail on at least a one of them). So, obviously this makes it problematic for setup.py to import things from twisted.python._release, either directly or indirectly via twisted.python.dist. There seem to be a few reasons the dependency goes in the direction it does:

  1. DistributionBuilder - Basically the core of SDistTwisted, and implemented in _release with all the other builders (and dependent itself on ManBuilder and DocBuilder). ManBuilder and DocBuilder may actually work on Windows (though I'm not positive) and I suspect they work fine on non-Linux POSIXs.
  2. makeAPIBaseURL and isDistributable - nothing really complicated here

The solution here may just be to make the skips in the tests for twisted.python._release more specific so that the parts of that module which need to be cross-platform are actually be tested across multiple platforms. I did really like the idea of having the entire module be Linux-only, though. It meant it was really easy to tell if some code that needed to be cross platform actually would be.

Some other minor points which I've taken care of (r27955):

  1. The multi-line import in setup.py could use () instead of \ now (yay, Python 2.4) and there's a stray period at the end of the second line
  2. There's some trailing whitespace near the data_files argument in setup.py
  3. The cmdclass parameter is still defined in terms of sdist_twisted instead of SDistTwisted.
  4. twisted/python/dist.py has a multi-line import that could change too.
  5. assertEquals yay, assertEqual boo. fail* boo!
  6. Some uses of path.open("r").read() (in test_release.py) would be better as path.getContent() (which will make sure the file is closed, too).

Some other small-ish issues that I didn't want to do myself:

  1. The NotImplementedError at the end of _stageFile is untested; neither is the non-EXDEV error case for the copyfunc call.
  2. twisted.python.dist.getDataFiles is now iterating over a list as it removes things from that list. Surely this will cause some problem.
  3. Still a "/bin" in ... in test_release.py
  4. The makeAPIBaseURL test could verify that the result is a str, or that it can be parsed as an URL or something like that.

  2010-01-11 19:33:37+00:00 changed by exarkun

Also, twisted.python.test.test_dist.GetDataFilesTests.test_directoryRecursion fails on Windows.

follow-up: ↓ 31   2010-01-12 00:25:25+00:00 changed by TimAllen

Man, I'd whined about this in #twisted, but never actually thought it through to its logical conclusion like that before. Twisted's current packaging system has the basic assumption that only Twisted devs will ever make a Twisted package, so the code doesn't need to be made generalised or portable, while distutils has the basic assumption that anyone anywhere should be able to make a package. These two assumptions are completely at odds. Trying to work around this conflict just leads to redundancy, trying to resolve it requires a fairly fundamental rework or rewrite of Twisted's packaging code.

I expect I'll have to think about this a while longer. Suggestions welcome.

in reply to: ↑ 30   2010-01-18 01:18:57+00:00 changed by glyph

Replying to TimAllen:

Man, I'd whined about this in #twisted, but never actually thought it through to its logical conclusion like that before. Twisted's current packaging system has the basic assumption that only Twisted devs will ever make a Twisted package, so the code doesn't need to be made generalised or portable, while distutils has the basic assumption that anyone anywhere should be able to make a package. These two assumptions are completely at odds. Trying to work around this conflict just leads to redundancy, trying to resolve it requires a fairly fundamental rework or rewrite of Twisted's packaging code.

This isn't quite right.

distutils' code is generalized so that anyone anywhere should be able to make a distribution; Twisted's code is specialized because only Twisted developers need to make a release. Distutils doesn't do most of the stuff that Twisted's release code needs to, and it doesn't need to be taught to. Even twisted devs need to be able to take the output of the release process and build packages on other platforms; the Windows toolchain, for example, is a big pain in the ass to get working on other platforms, and even if we could get it to work it would be impossible to test.

The thing that needs to be able to do 'sdist' can't depend on the code which does the release-building, but it can depend on work that is done by the thing that makes the release, including building API documentation, incrementing version numbers, building manpages, generating LaTeX ouptut (which, if I understand correctly, is the main reason that this code is platform-specific), and building news files.

Now that I understand the distinction, my suggestion is to put the new distutils code into twisted.python.dist (well, actually, I'd prefer twisted.python._dist), and move any functions that distutils might want to call from _release (which may be entirely platform-specific) to _dist (which must be supported cross-platform). I strongly suspect that most of them would not be a significant amount of effort to support cross-platform since I believe they're mostly must moving files around, not spawning subprocesses.

How does that sound?

Note: See TracTickets for help on using tickets.