RPMs fail to build on RHEL 4

[shieldszero]

On Red Hat EL4, Twisted 2.2.0 fails to build into RPMs with "python setup.py all bdist_rpm". Here are workarounds for two problems:

1. The detectExtensions hack in Twisted-2.2.0/twisted/python/dist.py setup() causes the build to fail. The build proceeds if this is commented out.

2. Except for the core and Conch, the components all have names in setup.py of the form "Twisted Something". This causes the RPM spec file to be invalid, as RPM package names cannot have spaces. A fix is to change the names to "TwistedSomething".

[glyph]

These sound like bdist_rpm bugs, not Twisted bugs.

[TimAllen]

Here in Fedora 6 (with Python 2.4.4) and SVN revision 23447 (after v8.0.1) I can't build RPMs because it tries to include the file twisted/test/generator_failure_tests.py in the RPM, which is a file of tests that require Python2.5 features.

Unfortunately, one of the features it tests is the ability to use yield inside try/except, so Python dies with a SyntaxError on import, even if you don't call any of the code defined in the file.

Is there some way to automatically exclude that file from the install on Python 2.4?

The relevant portion of the bdist_rpm output is:

writing byte-compilation script '/tmp/tmpy6X63z.py'
/usr/bin/python -O /tmp/tmpy6X63z.py
  File "/usr/lib/python2.4/site-packages/twisted/test/generator_failure_tests.py", line 66
    yield
         ^
SyntaxError: invalid syntax
removing /tmp/tmpy6X63z.py

[exarkun]

If there were a way to exclude the file, wouldn't it most likely be a feature of bdist_rpm? The Debian packages currently run into this problem as well. For sdist_dsc (a somewhat new distutils plugin), it's possible to write a configuration file which, among other things, allows files to be excluded. Is there anything similar for bdist_rpm?

[TimAllen]

After:

• Deleting the offending file
• Finding all the other places bdist_rpm caches copies of the file, and deleting them too (hint: /var/tmp/Twisted-8.0.1-1-buildroot)
• Running python setup.py bdist_rpm with the RPM workarounds mentioned  here

...I can confirm that it successfully builds an RPM.

Unfortunately, the built RPM doesn't contain any .so files, even though the tarball from "python setupy.py bdist" includes them. I guess that's better than nothing, though.

[TimAllen]

More news from the front. After taking a clean checkout and:

• Creating a file called MANIFEST.in next to setup.py with the following contents:

  exclude twisted/test/generator_failure_tests.py
  

• Creating a file called setup.cfg next to setup.py with the following contents:

  [install]
  optimize = 1
  

...then I can cleanly build an RPM by running "python setup.py bdist_rpm" with no further configuration (still without .so files, sadly).

[TimAllen]

(note that the MANIFEST.in file actually configures the sdist command, which bdist_rpm uses internally, but which is also used for other things like, well, building source-distributions)

[TimAllen]

OK, I've got it working. The issue is that bdist_rpm works by running sdist, then creating a .spec file that turns the resulting tarball into an RPM. Unfortunately, Twisted's extensions are declared in twisted/topfiles/setup.py, which isn't marked as part of the package. Hence, sdist strips out the extension definitions, and they aren't available to the RPM build process.

Applying the following patch to a fresh checkout of Twisted makes bdist_rpm work out-of-the box for me:

diff --git a/MANIFEST.in b/MANIFEST.in
new file mode 100644
index 0000000..2d8ade4
--- /dev/null
+++ b/MANIFEST.in
@@ -0,0 +1,7 @@
+# Exclude source-files that require Python 2.5 to parse, so we can build RPMs
+# on Python 2.4.
+exclude twisted/test/generator_failure_tests.py
+
+# Include TwistedCore's extension-definitions in the sdist package, so they'll
+# be available for bdist_rpm to read.
+recursive-include twisted/topfiles *
diff --git a/setup.cfg b/setup.cfg
new file mode 100644
index 0000000..2b3310b
--- /dev/null
+++ b/setup.cfg
@@ -0,0 +1,7 @@
+[install]
+; Deliberately create .pyo files during 'setup.py install' so that it records
+; them as installed. The alternative is accidentally creating .pyo files and not
+; recording them as installed, which breaks 'setup.py bdist_rpm' on modern
+; RedHat-alike distros. (see
+; https://bugzilla.redhat.com/show_bug.cgi?id=198877#c1)
+optimize = 1

It might screw up Twisted's standard packaging and distribution system (whatever that is), but the key point is to control what gets added to the sdist tarball.

The setup.cfg patch would be nice, but is a work-around for a bug that bdist_rpm seems to have on most modern RedHat-alike distros, and I wouldn't be too annoyed if you left that bit out.

[exarkun]

Want to think about this a bit?

[TimAllen]

According to  this mailing-list post, the above is insufficient for a proper RPM: there should also be some kind of post-install script that runs:

from twisted.plugin import IPlugin, getPlugins
list(getPlugins(IPlugin))

(as seen at the bottom of  this page about Twisted's plugin system).

I expect the best approach would be to hook into distutils' "install" target, but I'm not really sure how.

[dripton]

TimAllen, the usual way to run a post-install script after bdist_rpm is to add a post-install line to the [bdist_rpm] section of setup.cfg, like this:

[bdist_rpm]
requires = foo, bar
build_requires = baz
post-install = relative/path/to/my/post/install/script

[TimAllen]

Replying to dripton:

TimAllen, the usual way to run a post-install script after bdist_rpm is to add a post-install line to the [bdist_rpm] section of setup.cfg

I wanted to make it generic - the code in my previous comment ought to be run after twisted is installed by any mechanism (install, bdist_rpm, bdist_win32), not just RPMs. However, distutils seems to lack such a generic post-install system, and I don't think Twisted's setup.py is the place to be writing a new package-manager.

Twisted devs, would you be willing to add, say, a top-level redhat/ or rpm/ directory for storing a post-install script?

[exarkun]

I don't see any reason not to have such a directory as a place for installation utilities/configuration which needs to be maintained as part of Twisted. Ideally anything that gets written should be testable in an automated way. eg, it would be great if one of our redhat (well, fedora, that's the same, right?) machines could build rpms on each trunk commit, install them, run the tests, and report the results, so that we know that the package actually works.

[thijs]

See related #2966

[glyph]

I'm not working on this.

[glyph]

I mean, chris isn't.

[jafo]

I believe this should be fixed in r26608.

[TimAllen]

I just updated to r26608 and tried build an RPM, and it seems that most of the infrastructure is there, which is good.

However, I believe this bug still isn't fixed - the RPM didn't successfully build on my Python 2.4 workstation (and hence I expect it won't build on RHEL 5) because the Python 2.5-specific "generator_failure_tests.py" was not excluded. You should update the version check setup.py to apply that monkey-patch for 2.4 as well as 2.3.

Also, I see the "_unpackaged_files_terminate_build" fix is only being applied for older Python versions... this seems a bit odd: as I understand it, this disables a safety check added by versions of RPM newer than 4.something - so only enabling it for older distributions seems the Wrong Thing.

Lastly, it seems RPMs built this way needlessly install the C source to Twisted's extensions into /usr/twisted/. This is annoying but not actively harmful, I guess.

Can this ticket be re-opened?

[exarkun]

(In [26609]) Revert r26608 - some features not implemented

Reopens #1696

[exarkun]

(In [26610]) Branching to 'make-some-rpms-1696'

[dreid]

(In [26612]) Branching to 'make-some-rpms-1696'

[exarkun]

Okay, re-opened. :) I pointed out your (TimAllen) last comment to jafo and hopefully he'll figure out what else needs to be do. The changes he made are in the branch linked above for now.

[jafo]

r26618 branches/make-some-rpms-1696

I've fixed these issues that you pointed out and tested it on CentOS 4 and 5. Can you verify it? Thanks.

[TimAllen]

Looks good - it certainly resolves my complaints from before, and I'll quite happily use this branch as-is to build RPMs to keep my production systems up-to-date.

I did just notice one other thing, though: among the scripts installed by the resulting RPM are /usr/bin/build-apidocs, /usr/bin/build-tarballs and /usr/bin/change-versions, which I don't think are useful outside a Twisted SVN checkout. They're not causing any problems, but I can imagine somebody tripping over them someday.

[jafo]

r26625 make-some-rpms-1696

I've excluded the bin/admin scripts from the sdist. Anything else I should do?

[TimAllen]

I tried the new revision on a CentOS 4 VM I had lying around, and it built perfectly. I couldn't install it as I didn't have an RPM of zope.interface handy, but the Twisted RPM contained everything I'd hoped it would.

The real test will be 'can an RPM be built from a Twisted release tarball' but I don't know how to make those so I can't test.

This branch is a definite improvement over the current state of the trunk, so as far as I'm concerned the make-some-rpms-1696 branch is good to go.

[jafo]

Committed 26646 which updates the .spec file so that "rpmbuild -ta" will allow the tar file to be built into an RPM. Note, this requires that the tar file be a .bz2 format, and extract a directory named "Twisted-$VERSION", and the spec file in it include the correct version. I have tested it on CentOS 4 and 5 systems.

Sean

[thijs]

Replying to TimAllen:

Unfortunately, one of the features it tests is the ability to use yield inside try/except, so Python dies with a SyntaxError on import, even if you don't call any of the code defined in the file. Is there some way to automatically exclude that file from the install on Python 2.4? The relevant portion of the bdist_rpm output is: {{{ writing byte-compilation script '/tmp/tmpy6X63z.py' /usr/bin/python -O /tmp/tmpy6X63z.py File "/usr/lib/python2.4/site-packages/twisted/test/generator_failure_tests.py", line 66 yield ^{SyntaxError: invalid syntax removing /tmp/tmpy6X63z.py }}}}

This issue is logged in ticket #3243

[thijs]

Replying to thijs:

This issue is logged in ticket #3243

And was closed as a duplicate of this ticket.

[jafo]

TimAllen: Any objection to me calling this fixed?

[TimAllen]

No objections from me.

[jafo]

(In [26654]) Merge twisted-make-some-rpms-1696: Resolving issues with RPM builds.

This branch fixes several issues with RPM building, including using "python setup bdist_rpm", and building an RPM from the tar file.

Author: Sean Reifschneider Reviewer: Tim Allen Fixes: #1696

[glyph]

(In [26655]) Revert r26654 - accidentally mis-merged (added files were not committed)

Reopens #1696

[glyph]

As long as this has been reopened, a couple of additional review notes:

1. We've worked long and hard to eliminate lots of code from setup.py and put it in a more testable location; specifically, twisted.python.dist. See, for example, build_py_twisted, install_data_twisted, build_scripts_twisted in that module.
2. Some unit tests, even if somewhat cursory, would be nice. We definitely don't want to try and have tests that make sure that RPMs build and install correctly (that's a buildbot's job) but, for example, a test for the code that builds the FileList and makes sure it excludes the file we think it should exclude would be good.
3. the things which are in class-level comments should really be in docstrings.
4. The comments in the spec file should be a bit more specific. For example, "This is to work around an issue with some versions of RPM." What issue? What versions?
5. Why do both the spec file and the sdist command have code that elides the generator failure tests? Are these different methods of creating an RPM? If so it would be good to have a comment at the top of the spec file saying why one would use one method over the other. It would be good to invoke the same code from both places so that we only have to update one location to change the list of RPM-unfriendly files.
6. It would be nice to avoid polluting sdist with RPM-specific concerns. Especially given that the sdist modification here has the unintended side-effect of making an sdist tarball Python-version specific; e.g. if you make an sdist tarball with python 2.4, it will no longer work on python 2.5. We don't use sdist to do our releases, but it would be nice to have that option later.

I'm open to doing some of this stuff in separate tickets, but I figure that at least the part where we move the code to an importable location so we can include it in our unit tests would be good.

[jafo]

To address your questions:

1. From a cursory look, it seems like it should be possible to move these changes into the twisted.python.dist module.
2. Seems to me that the right way to test this is to require that the unit tests have access to the version of Python right before and right after yeild was added, and run the tests through there. Are there any problems with requiring these specific versions of Python for the unit tests? In any case, I would say this is a prime suspect for a separate ticket.
3. No problem.
4. The issue is discussed in this URL (mentioned above):  https://bugzilla.redhat.com/show_bug.cgi?id=198877#c1 In short, at some point the RPM build process changed such that files installed into the destination directory but not included in the package were fatal errors. I don't know exactly which versions are impacted. However, since then it's been a struggle, particularly when dealing with RPM at arms-length as with setup.py, to get the RPM build process to do the right thing in some situations. So a work-around is to reset it back to it's old behavior. Which is fine, as long as we're sure that the installed but unpackaged files are legitimately not packaged.
5. They both do it because they both need to do it. They are indeed different ways to build the RPM. One allows you to build the RPM by doing "rpmbuild -ta twisted-tar-file" without extracting it. The other allows you to build it by extracting the tar file and doing "python setup.py bdist_rpm". Does twisted *NEED* both build mechanisms? Possibly not. It's probably best to rely on bdist_rpm and get rid of the spec file.
6. It's not entirely fair to say that the 2.4 sdist won't work... It's just that it will be missing one of the tests. The real issue is that twisted's source is syntactically incorrect for Python versions which do not support the "yield" statement. We could entirely get around this issue if Twisted didn't include files that are syntactically incorrect in some supported Python versions. The reason this comes up is that the RPM build process will try to compile all the Python code it finds for inclusion in the package, which requires parsing all the files.

Perhaps the syntactically incorrect code could be put in an "exec()" so that it's only evaluated on a platform for which it's syntactically correct? Realize, this is in a test not the core library, so things like the performance impact don't matter as much.

Thoughts?

[glyph]

Replying to jafo:

To address your questions: 1. From a cursory look, it seems like it should be possible to move these changes into the twisted.python.dist module.

Great.

2. Seems to me that the right way to test this is to require that the unit tests have access to the version of Python right before and right after yeild was added, and run the tests through there. Are there any problems with requiring these specific versions of Python for the unit tests? In any case, I would say this is a prime suspect for a separate ticket.

No, that's no problem. As you can see on  the buildbot, we already have builders with all supported versions of Python, including 2.3 and 2.4.

Tests typically come along with the same ticket that require them except in the very rare cases wehre new infrastructure is required to support testing something; in which case we usually write tests anyway, which use fakes or mocks and defer the infrastructure. Was it the tests or the multi-python-version infrastructure you were suggesting for another ticket?

3. No problem.

Great.

4. The issue is discussed in this URL (mentioned above):  https://bugzilla.redhat.com/show_bug.cgi?id=198877#c1 In short, at some point the RPM build process changed such that files installed into the destination directory but not included in the package were fatal errors. I don't know exactly which versions are impacted. However, since then it's been a struggle, particularly when dealing with RPM at arms-length as with setup.py, to get the RPM build process to do the right thing in some situations. So a work-around is to reset it back to it's old behavior. Which is fine, as long as we're sure that the installed but unpackaged files are legitimately not packaged.

Thanks for the detailed explanation. But, I should have phrased my request more specifically :). It wasn't so much that I wanted an answer as I wanted an answer encoded along with the comment, so that the future maintainers looking at this file would know what you were talking about. If you could just copy and paste this into the comment that would be great; or, if we're going to keep both files, copy and paste it into the python file and reference it from the spec file.

5. They both do it because they both need to do it. They are indeed different ways to build the RPM. One allows you to build the RPM by doing "rpmbuild -ta twisted-tar-file" without extracting it. The other allows you to build it by extracting the tar file and doing "python setup.py bdist_rpm". Does twisted *NEED* both build mechanisms? Possibly not. It's probably best to rely on bdist_rpm and get rid of the spec file.

Deleted code is code that doesn't need tests. I like the idea of eliminating redundant stuff.

6. It's not entirely fair to say that the 2.4 sdist won't work... It's just that it will be missing one of the tests. The real issue is that twisted's source is syntactically incorrect for Python versions which do not support the "yield" statement. We could entirely get around this issue if Twisted didn't include files that are syntactically incorrect in some supported Python versions. The reason this comes up is that the RPM build process will try to compile all the Python code it finds for inclusion in the package, which requires parsing all the files.

Sounds like we could write a test case which would just recursively descend the 'twisted' hierarchy and try parsing all the files, similar to twisted.test.test_doc.DocCoverage.

Perhaps the syntactically incorrect code could be put in an "exec()" so that it's only evaluated on a platform for which it's syntactically correct? Realize, this is in a test not the core library, so things like the performance impact don't matter as much.

This sounds like a radically simpler approach which could remove a lot of the code in this branch and get the same effect. I like it a lot.

Thanks again for slogging through this with us!

[exarkun]

Sounds like we could write a test case which would just recursively descend the 'twisted' hierarchy and try parsing all the files, similar to twisted.test.test_doc.DocCoverage.

This sounds like it would be slow and I don't think we really need it. We know that this file is not syntactically correct. We chose to put it there. If we change our minds about it, we can fix that. Having a test point it out to us is redundant (and redundancy is bad when it costs time in the test suite),

[glyph]

Replying to exarkun:

This sounds like it would be slow and I don't think we really need it. We know that this file is not syntactically correct. We chose to put it there. If we change our minds about it, we can fix that. Having a test point it out to us is redundant (and redundancy is bad when it costs time in the test suite),

Point taken; I agree. I need to stop doing this sort of general-solution-to-a-specific-problem stuff. We don't need another twisted.test.test_modules.

[exarkun]

#3988 was a duplicate of this.

Anyone feel like giving this the last bit of a push to resolve it?

[TimAllen]

I have attached "make-some-rpms-1696-2.patch" based on today's SVN trunk. The patch is a hodge-podge of the code I reviewed in comment:36, combined with some of Glyph's review notes in comment:43 and my own increased experience since the last time I tackled this issue.

There's three main parts to this patch:

• twisted/test/generator_failure_tests.py now has the problematic syntax wrapped in exec, and a skip attribute is added to the class to explain why these tests are not run on older versions of Python.
• A variety of new files are added to make the RPM building go smoothly: rpm-post-install creates the IPlugin cache, setup.cfg hooks up the post-install script and works around the unpackaged-files error, and MANIFEST.in papers over the difference between what's included in an SVN checkout, and what's packaged by setup.py sdist.
• Finally, admin/twisted.spec is removed, since I aim for bdist_rpm to be the One True Way.

I have tested this on Fedora 6 with Python 2.4, and hence I expect it will work on CentOS/RHEL 5 as well. Testing on CentOS/RHEL 4 and some distro with Python 2.5 would be appreciated.

There are no added unit tests, because the only Python file I've changed is a test suite anyway.

The biggest known problem is that the C source to the various optional extensions is packaged into the resulting RPM - it needs to be packaged in the output of sdist, but I don't know how to exclude it from bdist, but the extra dozen-or-so files aren't hurting anything.

[TimAllen]

I've just noticed the changes above don't mark the resulting RPM as depending on zope.interface (or as the RHEL packagers named it, python-zope-interface). I've modified my local copy of setup.py to add that, and I'm running the Twisted unit-tests in a CentOS 4 VM right now.

[TimAllen]

Yet another attempt at making bdist_rpm work, with dependencies.

[TimAllen]

I have updated the attached patch. Since comment:47,

• I have added RPM dependencies on "python >= 2.3" and "python-zope-interface >= 3.0.1" since those are the versions that seem to ship with CentOS 4 (feel free to revise up or down accordingly).
• I have tested the resulting RPM in a VM running CentOS 4 x86_64. It installs well enough, but trying to run the unit-tests against the installed version produces a lot of failures. Judging by ticket #3787, it seems testing an installed copy of the Twisted is not a supported configuration, so I'll ignore this.
• I have tested the twisted checkout in the same VM, but not installed - this works fine as long as I don't run it on an NFS-mounted filesystem, otherwise mtime-related tests break (another unsupported configuration, judging from #1393 and #3215).
• I have added code to a Python file (setup.py) but there are still no unit tests as I'm not sure what such tests would look like, beyond buildbot-style 'build an RPM, see if it installs'.

[glyph]

I apologize yet again for the insanely long review latency on this patch. Not many reviewers have an RPM build system set up, so this ticket frequently gets skipped.

Replying to TimAllen:

I have updated the attached patch. Since comment:47, * I have added RPM dependencies on "python >= 2.3" and "python-zope-interface >= 3.0.1" since those are the versions that seem to ship with CentOS 4 (feel free to revise up or down accordingly).

Are these actually required in order to get the package to build?

I have tested the resulting RPM in a VM running CentOS 4 x86_64. It installs well enough, but trying to run the unit-tests against the installed version produces a lot of failures. Judging by ticket #3787, it seems testing an installed copy of the Twisted is not a supported configuration, so I'll ignore this.

It really should be a supported configuration, but you're correct that that's another issue. However, you should make sure that the list of failures you get is the expected list.

I have tested the twisted checkout in the same VM, but not installed - this works fine as long as I don't run it on an NFS-mounted filesystem, otherwise mtime-related tests break (another unsupported configuration, judging from #1393 and #3215).

Great. Maybe you want to tackle some of those tickets, too :).

I have added code to a Python file (setup.py) but there are still no unit tests as I'm not sure what such tests would look like, beyond buildbot-style 'build an RPM, see if it installs'.

There are three answers I can think of:

One is, exactly as you propose, to set up a buildbot which actually builds an RPM and tries to install / remove it. This ultimate integration test functionality would be great to have. I hope someone will provide it, since we're not going to really be confident in supporting RPM-building until we have something like that :). In lieu of such a buildbot (which I don't think should be necessary to close this ticket), it would be nice if you attached some logs to this ticket so reviewers can see what RPM does with the changes in question.

The second answer is to provide high-level unit-testing of as much functionality as possible; this would involve moving the 'options' section to a function like twisted.python.dist.getOptions, as with many of the other functions, and adding a simple unit test to verify that it returns some correct values. Despite the fact that there isn't much to the current code, I think that this would be the right place to put this new declaration, since it follow the current convention, and it will prevent this discussion from happening every time we want to add a new bit of RPM metadata (like, for example, a suggestion that the user install PyCrypto or PyOpenSSL).

The third option would be to add some tests that verify the problematic behavior being modified here to actually allow RPMs to build. Right now we have 2.3 and 2.4 buildbots available,  which are passing, but according to the comment that the patch adds to generator_failure_tests.py, they should be failing. An integration test in the style of twisted.test.test_import, which attempted to invoke compileall.compile_dir, would cause those buildbots to fail, and the change in this patch to cause them to succeed. This would verify that future changes wouldn't cause RPMs (or any other package system which requires all .py files to be compilable with the current python version) to stop building.

Finally, two unrelated review points:

1. rpm-post-install should really not be cluttering up the top-level directory. It should be placed into admin/, most likely; although given that it is equally useful as a Debian post-installation hook we may want to consider putting it somewhere that it will actually be included with the distribution.
2. The MANIFEST.in might interfere in some way with our existing release process; a buildbot should verify this, but it would be great if you can track down radix and make sure that there's no other unintended consequences. (I am cagey around anything related to distutils.)

Thanks again for pursuing this ticket.

[TimAllen]

Replying to glyph:

Replying to TimAllen:

I have updated the attached patch. Since comment:47, * I have added RPM dependencies on "python >= 2.3" and "python-zope-interface >= 3.0.1" since those are the versions that seem to ship with CentOS 4 (feel free to revise up or down accordingly).

Are these actually required in order to get the package to build?

These are run-time dependencies, not build dependencies. I guess it's technically possible to install the Twisted package without Python or zope.interface, but I can't imagine anybody ever doing it except by accident.

I have tested the resulting RPM in a VM running CentOS 4 x86_64. It installs well enough, but trying to run the unit-tests against the installed version produces a lot of failures. Judging by ticket #3787, it seems testing an installed copy of the Twisted is not a supported configuration, so I'll ignore this.

It really should be a supported configuration, but you're correct that that's another issue.

At least with current trunk, and without PyCrypto, PyASN1 or PyPAM available, the only test that fails on an installed Twisted is covered by ticket #3101.

However, you should make sure that the list of failures you get is the expected list.

Thanks for prodding me to check this - with the patch currently attached, running trial against an RPM-installed Twisted generates 8 failures and 85 errors; mostly from attempts to read data-files that weren't scooped up into the package. Most of these were test-data files in test directories, but it seems Lore has its own bunch of data-files too (DTDs and entities) - my next patch will address all these.

For the record, this sort of thing would prevents "./setup.py sdist" from working too, but I assume that's not part of the Twisted release process.

I have tested the twisted checkout in the same VM, but not installed - this works fine as long as I don't run it on an NFS-mounted filesystem, otherwise mtime-related tests break (another unsupported configuration, judging from #1393 and #3215).

Great. Maybe you want to tackle some of those tickets, too :).

Well, I filed #3215 and included a patch, #1393 I left a 'WORKSFORME' comment on. :)

It seems there's a few more tests been added since that fail on NFS, maybe I should file tickets for those.

One is, exactly as you propose, to set up a buildbot which actually builds an RPM and tries to install / remove it. This ultimate integration test functionality would be great to have. I hope someone will provide it, since we're not going to really be confident in supporting RPM-building until we have something like that :). In lieu of such a buildbot (which I don't think should be necessary to close this ticket), it would be nice if you attached some logs to this ticket so reviewers can see what RPM does with the changes in question.

Hm... logs of what? The RPM-building? RPM-installation? The output of "trial twisted" on an RPM-installed copy of Twisted?

The second answer is to provide high-level unit-testing of as much functionality as possible; this would involve moving the 'options' section to a function like twisted.python.dist.getOptions, as with many of the other functions, and adding a simple unit test to verify that it returns some correct values. Despite the fact that there isn't much to the current code, I think that this would be the right place to put this new declaration, since it follow the current convention, and it will prevent this discussion from happening every time we want to add a new bit of RPM metadata (like, for example, a suggestion that the user install PyCrypto or PyOpenSSL).

I've done this, but I'm not sure if twisted.python.dist.get_setup_args() would be a better place. Also, I'm not sure what the deal is with building sub-packages - in an ideal world, RPMs built of sub-packages would automatically have a dependency on an RPM of the associated release of twisted-core. It doesn't look like the machinery in setup.py is really built to accommodate such things, however, so I'll leave it.

The third option would be to add some tests that verify the problematic behavior being modified here to actually allow RPMs to build. Right now we have 2.3 and 2.4 buildbots available,  which are passing, but according to the comment that the patch adds to generator_failure_tests.py, they should be failing.

They're not failing because test_failure only imports the generator failure tests if the Python environment is 2.5 or greater - trial doesn't find the tests, but the RPM build process does.

Since the generator-failure tests are now import-safe for older versions of Python, I've moved them back into test_failure and removed the now-empty generator_failure_tests.py.

I've also added twisted/test/test_compile.py that calls compileall.compile_dir() as you suggest.

Finally, two unrelated review points: 1. rpm-post-install should really not be cluttering up the top-level directory. It should be placed into admin/, most likely; although given that it is equally useful as a Debian post-installation hook we may want to consider putting it somewhere that it will actually be included with the distribution.

I've moved it into admin/, but I welcome alternative suggestions.

2. The MANIFEST.in might interfere in some way with our existing release process; a buildbot should verify this, but it would be great if you can track down radix and make sure that there's no other unintended consequences. (I am cagey around anything related to distutils.)

Where might I find radix?

[TimAllen]

Address (some of) Glyph's review comments.

[TimAllen]

I've attached a patch which represents what I have so far, just as a checkpoint.

Before I submit another patch for review, I'd appreciate it if Glyph could respond to some of my questions about his questions in comment:52, so assigning this ticket to him (hope that's OK).

[glyph]

Replying to TimAllen:

Replying to glyph:

Replying to TimAllen:

I have updated the attached patch. Since comment:47, * I have added RPM dependencies on "python >= 2.3" and "python-zope-interface >= 3.0.1" since those are the versions that seem to ship with CentOS 4 (feel free to revise up or down accordingly).

Are these actually required in order to get the package to build?

These are run-time dependencies, not build dependencies. I guess it's technically possible to install the Twisted package without Python or zope.interface, but I can't imagine anybody ever doing it except by accident.

Well, the bug here is that the Twisted RPM won't build :). I was trying to understand if these are miscellaneous fixes bundled in with the change that makes it build, or somehow crucial to make it do so. I'd rather have a separate ticket for the lack of dependencies, even if it's resolved by the same commit, so we can track it.

I have tested the resulting RPM in a VM running CentOS 4 x86_64. It installs well enough, but trying to run the unit-tests against the installed version produces a lot of failures. Judging by ticket #3787, it seems testing an installed copy of the Twisted is not a supported configuration, so I'll ignore this.

It really should be a supported configuration, but you're correct that that's another issue.

At least with current trunk, and without PyCrypto, PyASN1 or PyPAM available, the only test that fails on an installed Twisted is covered by ticket #3101.

Cool. I mean, let's install those and fix the rest, too ;-).

However, you should make sure that the list of failures you get is the expected list.

Thanks for prodding me to check this - with the patch currently attached, running trial against an RPM-installed Twisted generates 8 failures and 85 errors; mostly from attempts to read data-files that weren't scooped up into the package. Most of these were test-data files in test directories, but it seems Lore has its own bunch of data-files too (DTDs and entities) - my next patch will address all these. For the record, this sort of thing would prevents "./setup.py sdist" from working too, but I assume that's not part of the Twisted release process.

This is good to hear. I bet there's a ticket about sdist somewhere. If you wouldn't mind, can you search for it and link it here?

I have tested the twisted checkout in the same VM, but not installed - this works fine as long as I don't run it on an NFS-mounted filesystem, otherwise mtime-related tests break (another unsupported configuration, judging from #1393 and #3215).

Great. Maybe you want to tackle some of those tickets, too :).

Well, I filed #3215 and included a patch, #1393 I left a 'WORKSFORME' comment on. :) It seems there's a few more tests been added since that fail on NFS, maybe I should file tickets for those.

If you're interested in this, an NFS buildbot would also be good.

One is, exactly as you propose, to set up a buildbot which actually builds an RPM and tries to install / remove it. This ultimate integration test functionality would be great to have. I hope someone will provide it, since we're not going to really be confident in supporting RPM-building until we have something like that :). In lieu of such a buildbot (which I don't think should be necessary to close this ticket), it would be nice if you attached some logs to this ticket so reviewers can see what RPM does with the changes in question.

Hm... logs of what? The RPM-building? RPM-installation? The output of "trial twisted" on an RPM-installed copy of Twisted?

Since this ticket is about the RPMs failing to build, I was referring to logs of just the build. Just some evidence that others can examine that the build actually did work. (More importantly, archival evidence so that if we have this problem again in the future, we can see any potentially interesting details of the setup which got the successful build that might not seem relevant now.)

The second answer is to provide high-level unit-testing of as much functionality as possible; this would involve moving the 'options' section to a function like twisted.python.dist.getOptions, as with many of the other functions, and adding a simple unit test to verify that it returns some correct values. Despite the fact that there isn't much to the current code, I think that this would be the right place to put this new declaration, since it follow the current convention, and it will prevent this discussion from happening every time we want to add a new bit of RPM metadata (like, for example, a suggestion that the user install PyCrypto or PyOpenSSL).

I've done this, but I'm not sure if twisted.python.dist.get_setup_args() would be a better place.

Hmm. Possibly. What you've done is okay (although again, it should be test_getOptionsForRPMs not test_getOptions_for_RPMs) but putting it into get_setup_args would avoid exposing yet another public API, and would make it more amenable to integration testing. Any particular reason not to put it there?

Also, I'm not sure what the deal is with building sub-packages - in an ideal world, RPMs built of sub-packages would automatically have a dependency on an RPM of the associated release of twisted-core. It doesn't look like the machinery in setup.py is really built to accommodate such things, however, so I'll leave it.

Let's not deal with that as a part of this ticket. RPMs that build at all should not be delayed for RPMs that build perfectly.

The third option would be to add some tests that verify the problematic behavior being modified here to actually allow RPMs to build. Right now we have 2.3 and 2.4 buildbots available,  which are passing, but according to the comment that the patch adds to generator_failure_tests.py, they should be failing.

They're not failing because test_failure only imports the generator failure tests if the Python environment is 2.5 or greater - trial doesn't find the tests, but the RPM build process does.

I understand that. What I'm saying is that you could add a test that compiles all .py files which can be discovered, as the RPM build process does.

Since the generator-failure tests are now import-safe for older versions of Python, I've moved them back into test_failure and removed the now-empty generator_failure_tests.py.

Since it has to be this way anyway, it sounds good to me.

I've also added twisted/test/test_compile.py that calls compileall.compile_dir() as you suggest.

Finally, two unrelated review points: 1. rpm-post-install should really not be cluttering up the top-level directory. It should be placed into admin/, most likely; although given that it is equally useful as a Debian post-installation hook we may want to consider putting it somewhere that it will actually be included with the distribution.

I've moved it into admin/, but I welcome alternative suggestions.

I was really hoping someone else would have one, because I don't particularly love admin/ (I was kind of hoping we could empty out that directory, actually, and it seemed like we were finally getting there) but I don't have any other suggestions.

2. The MANIFEST.in might interfere in some way with our existing release process; a buildbot should verify this, but it would be great if you can track down radix and make sure that there's no other unintended consequences. (I am cagey around anything related to distutils.)

Where might I find radix?

On IRC, or on the mailing list, or hey, now he's CC'd on this ticket.

[TimAllen]

Replying to glyph:

Replying to TimAllen:

Replying to glyph:

Replying to TimAllen:

Well, the bug here is that the Twisted RPM won't build :). I was trying to understand if these are miscellaneous fixes bundled in with the change that makes it build, or somehow crucial to make it do so. I'd rather have a separate ticket for the lack of dependencies, even if it's resolved by the same commit, so we can track it.

Filed ticket #4121 about dependencies.

At least with current trunk, and without PyCrypto, PyASN1 or PyPAM available, the only test that fails on an installed Twisted is covered by ticket #3101.

Cool. I mean, let's install those and fix the rest, too ;-).

CentOS 4 has a package called "python-crypto-2.0.1-1.el4.rf", but when I install it, Twisted Conch still skips tests saying "cannot run w/o PyCrypto". As far as I can tell, CentOS 4 doesn't have packages for gmpy, PyASN1 or PyPAM.

Looking closer at the PyCrypto error message, I see in twisted.conch.test.test_userauth:

try:
    import Crypto.Cipher.DES3, Crypto.Cipher.XOR
    import pyasn1
except ImportError:
    keys = None

[...]

    if keys is None:
        skip = "cannot run w/o PyCrypto"

...so it probably is the PyASN1 thing after all.

For the record, this sort of thing would prevents "./setup.py sdist" from working too, but I assume that's not part of the Twisted release process.

This is good to hear. I bet there's a ticket about sdist somewhere. If you wouldn't mind, can you search for it and link it here?

Incredibly, there doesn't seem to be - if I search for 'sdist' in Tickets, the only unresolved tickets are this one, #1533 and the unrelated #3912. #1533 is about supporting bdist_msi but apparently the issues aren't related to sdist being broken. If you think the 'fixing sdist' should be filed as a separate bug blocking this one, that sounds reasonable (even if the only motivation for fixing sdist is to get bdist_rpm working).

Since this ticket is about the RPMs failing to build, I was referring to logs of just the build. Just some evidence that others can examine that the build actually did work.

I've made logs of a successful-build from my branch, and an unsuccessful build from the trunk, which I shall hope I remember to attach after I finish writing this reply.

[...] this would involve moving the 'options' section to a function like twisted.python.dist.getOptions, as with many of the other functions, and adding a simple unit test to verify that it returns some correct values.

I've done this, but I'm not sure if twisted.python.dist.get_setup_args() would be a better place.

Hmm. Possibly. What you've done is okay (although again, it should be test_getOptionsForRPMs not test_getOptions_for_RPMs) but putting it into get_setup_args would avoid exposing yet another public API, and would make it more amenable to integration testing. Any particular reason not to put it there?

The main reason is that get_setup_args has a lot of delicate-looking logic for dealing with Twisted subprojects, and I'm not sure how the options key should interact with such things. If you'd like, I could remove this part of the patch entirely and leave these details to be resolved in #4121.

1. rpm-post-install should really not be cluttering up the top-level directory. It should be placed into admin/, most likely; although given that it is equally useful as a Debian post-installation hook we may want to consider putting it somewhere that it will actually be included with the distribution.

I've moved it into admin/, but I welcome alternative suggestions.

I was really hoping someone else would have one, because I don't particularly love admin/ (I was kind of hoping we could empty out that directory, actually, and it seemed like we were finally getting there) but I don't have any other suggestions.

In comment:14 I suggested a redhat/ or rpm/ directory, by analogy to debian/ (even if RPM doesn't have any special directory names like dpkg does). It seems better to re-use an existing directory than create a wholly new one, though.

2. The MANIFEST.in might interfere in some way with our existing release process; a buildbot should verify this, but it would be great if you can track down radix and make sure that there's no other unintended consequences. (I am cagey around anything related to distutils.)

Where might I find radix?

On IRC, or on the mailing list, or hey, now he's CC'd on this ticket.

I found him on IRC, wherein he had this to say (selectively quoted by me):

16:38 < radix> Screwtape: anyway, about MANIFEST.in, I doubt the addition of
    such a file would break anything that I care about
16:39 < radix> Screwtape: On the other hand, I think that the inclusion of a
    MANIFEST.in file is an unfortunate thing, because it's generally full of
    pointless or redundant data
16:49 < radix> Screwtape: but basically, if we include a MANIFEST.in file, it
    *will* get out of date
16:50 < Screwtape> radix: Presumably, unless somebody supplies a buildbot that
    actually tests the RPM installation process. ;)
16:50 < radix> maybe. And if we get that far, I will argue against MANIFEST.in
    purely on the grounds that it is bad software design :)

So, I guess that's a -1 from radix on my current approach.

Alternatives I can think of include:

• Instead of defaulting to exclusion and selectively including things, just include everything in twisted/. Sure, some crud would get packaged along with the required files, but it shouldn't actually break anything.
• Refactor twisted.python._release.DistributionBuilder and split out the code that generates the list of files to be packaged from the code that rebuilds documentation and writes a tarball. DistributionBuilder can use the code to build a tarball, and setup.py can use it to write out a fresh MANIFEST.in at every invocation (I don't know of a way to hook into the 'sdist' command specifically).

I imagine this work should probably be done in a separate 'make sdist work' ticket as a precondition for this ticket. How does that sound?

[TimAllen]

A log of "setup.py bdist.rpm" as it runs on the trunk.

[TimAllen]

A log of "setup.py bdist.rpm" as it runs with my changes applied.

[TimAllen]

I have filed ticket #4138 about making sdist work in a maintainable fashion, and attached a proof-of-concept patch to provoke discussion.

Once that ticket is resolved, I'll be able to supply a much smaller patch for this ticket.

[Screwtape]

The existing patch on this ticket gets around the problem of Python-2.4-incompatible source files by putting the offending syntax inside strings that are exec'd, however Glyph has filed #4182 about removing all exec'd strings from the tree. Once #4182 is resolved, we can modify this patch accordingly.

[TimAllen]

A minimal patch based on the current work in #4138

[TimAllen]

Glyph is making encouraging noises about the recent patches I've added to #4138, so I thought I'd investigate how much more work would be involved in building RPMs if #4138 were to be merged as it currently stands.

The attached patch has no tests or docstrings, and possibly needs the new code moved to more appropriate modules:

• Adds a setup.cfg that works around the long-standing Red Hat .pyo problem (taken from previous patches above)
• Replaces distutils' build_py command with one that has a hard-coded list of modules that require a particular version of Python. When building a distribution, it won't include modules that won't work on the target Python version.

Those two changes are enough to allow setup.py bdist_rpm to complete, although the resulting RPM still doesn't generate the plugin cache, and I'm getting some failures running the installed test suite. Still, it's half the size of the previous patch.

[TimAllen]

...it turns out that the vast majority of failing tests were caused by being unable to generate the plugin cache. After rebuilding it, there are only three failures (caused by Trial's path -> module code importing modules as "site-packages.twisted.trial.test.sample" instead of "twisted.trial.test.sample") and one error (#3101).

[TimAllen]

And *those* failures were caused by t.p.reflect.filenameToModuleName() getting confused, because some other Python package had dumped a __init__.py file into the site-packages directory. If I move that file aside, the only problem in the test suite is #3101.