Make tap2rpm work with modern versions of RPM.

[TimAllen]

I tried to run tap2rpm on a .tac file today, but rpm failed:

Starting build...
======================================================================
error: Legacy syntax is unsupported: copyright
error: line 5: Unknown tag: Copyright:  Unknown
Done with build...
======================================================================
Traceback (most recent call last):
  File "/usr/bin/tap2rpm", line 17, in ?
    tap2rpm.run()
  File "/usr/lib/python2.4/site-packages/twisted/scripts/tap2rpm.py", line 265, in run
    rpm_path = glob.glob(os.path.join(tmp_dir, 'RPMS', 'noarch', '*'))[0]
IndexError: list index out of range

It looks like rpm doesn't like the Copyright property in the .spec template that tap2rpm uses; according to  this (old) RPM manual the Copyright property is deprecated. The attached patch fixes it for me.

[TimAllen]

A patch to make tap2rpm work with modern RPM.

[radix]

Do you seriously use tap2rpm? I was hoping to eventually delete it.

[TimAllen]

Well, I don't yet, but I'm aiming to: I want a nice automated way to go from 'freshly-checked out source' to 'RPMs to automatically deploy on production machines'. I particularly like tap2rpm because the code is already written, and it's not my responsibility to maintain it.

Without tap2rpm, I think most people deploying Twisted-based software would eventually re-implement much the same thing themselves (as a literal tap2rpm script, or an installation shell-script, or a manual procedure or something), and all that duplication of effort seems a shame.

[radix]

Do you realize that tap2rpm doesn't include the actual code of your app? This is why I don't care about it very much; it's completely useless without some _other_ package which actually includes your application code.

[TimAllen]

Well, yes - I have a collection of Python source files, and a .tap file that uses them to create a server. distutils will package the Python source for me, and tap2rpm will package the .tap.

In an ideal world, there'd be some kind of distutils extension or wrapper to do all this; until such a thing appears, distutils and tap2rpm together are the 80% solution.

[radix]

Jerub said he'd be able to take a look at this at some point, since he actually uses a RPM-based operating system.

[exarkun]

I've verified that this patch changes tap2rpm from choking and dying early on to actually producing two .rpm files.

Since I could verify this on Ubuntu using tools natively packaged for that platform, I think it'd be reasonable to add some kind of automated test for this. It should skip when the requirements aren't present, of course.

[TimAllen]

I've attached another patch, which incorporates the previous one and also includes a fairly basic test suite that tests simple invocation and a few of the more useful-looking command-line switches.

If tap2rpm is to be supported, there's more changes that need to be done on top of these changes, and probably in a separate ticket:

• By default, the DESCRIPTION tag says "Automatically created by tap2deb", not "tap2rpm".
• The command-line parser defines an "unsigned" flag that's never used.
• tap2rpm dies horribly if asked to package a .tap outside the current directory.
• tap2rpm uses os.system() and hands it un-sanitized user input.

I can think of a number of nice-to-have upgrades too:

• Allow Requires/Provides/Obsoletes to be set, so tap2rpm-built RPMs can be managed with yum.
• Read settings from the tap/tac file, rather than requiring command-line arguments.
• Delete the commented-out zsh_* command-line options, or fix them.
• Make tap2rpm run without printing all the RPM output as well.

...but that's all in the future, and I think this patch provides a good base to work from.

[glyph]

This patch actually looks like it's in pretty good shape! A few minor issues:

1. Test docstrings should never say "should work". Be specific about what should work, and why. "Should produce an RPM which...", etc. Looking at the actual code which does the verification, it looks like you're definitely verifying useful things about the RPM output, not just blindly invoking the build and noticing it doesn't raise an exception, so just describe what got verified.
2. The Twisted coding standard is at odds with PEP-8 in some ways; in particular, method names lookLikeThis, not_like_this. Underscores have a specific meaning within Twisted; they're reserved to indicate what type a method is for dispatch purposes. For example, test methods are test_foo and HTTP method handlers are renderHTTP_FOO. So, rather than test_protocol_override, your methods should be entitled test_protocolOverride, etc. Methods that don't have a special defined dispatch type should only use underscores as the 'private' prefix. For example, _query_rpm_tags should be _queryRPMTags.
3. Another minor coding-standard thing; we prefer 2 blank lines between methods, 3 between classes.
4. Can you file tickets for some of the other issues that you mention, which link back to this ticket?

Thanks again for helping to get our Red Hat support into better shape; we'll try to get better about these review latencies!

[TimAllen]

Replying to glyph:

1. Test docstrings should never say "should work". Be specific about what should work, and why. "Should produce an RPM which...", etc. Looking at the actual code which does the verification, it looks like you're definitely verifying useful things about the RPM output, not just blindly invoking the build and noticing it doesn't raise an exception, so just describe what got verified.

It took me a while to figure out how to phrase it in one line, but I changed the "should work" docstring to be "Calling tap2rpm should produce an RPM and SRPM with default metadata."

2. The Twisted coding standard is at odds with PEP-8 in some ways; in particular, method names lookLikeThis, not_like_this. Underscores have a specific meaning within Twisted; they're reserved to indicate what type a method is for dispatch purposes. For example, test methods are test_foo and HTTP method handlers are renderHTTP_FOO. So, rather than test_protocol_override, your methods should be entitled test_protocolOverride, etc. Methods that don't have a special defined dispatch type should only use underscores as the 'private' prefix. For example, _query_rpm_tags should be _queryRPMTags.

$EMPLOYER's in-house coding style is different from both PEP8 and Twisted, and sometimes I get whiplash when switching from one to another - thanks for picking up on it.

I've de-underscored things, except for the leading-underscore on helper methods and functions, the first underscore in "test_*" methods, the underscores in "RECORD_SEPARATOR" and "UNIT_SEPARATOR", the underscores in lambdas that mean "this function needs to accept a parameter but ignores it", and the tail on the "type_" parameter to _makeRPMs(), since "type" is a built-in and I couldn't think of a better name for "the value that is passed to tap2rpm's --type commandline parameter".

3. Another minor coding-standard thing; we prefer 2 blank lines between methods, 3 between classes.

I've put 3 blank lines between classes and top-level functions, and 2 blank lines between methods.

4. Can you file tickets for some of the other issues that you mention, which link back to this ticket?

• tap2rpm should support Requires/Provides/Obsoletes: ticket #4081
• Read settings from the tap/tac file: ticket #4082
• tap2rpm's default long description should mention tap2rpm: #4083
• Delete zsh_* command line options: #4084
• tap2rpm uses os.system() and blurts RPM output: #4085
• Unused "unsigned" flag: #4086
• tap2rpm can't handle .tac files outside the current directory: #4088

I'll attach an updated version of the patch.

[thijs]

1. trunk/twisted/scripts/tap2rpm.py needs a copyright header.
2. I would also get rid of line 4 and open a ticket for it:

#  TODO: need to implement log-file rotation

3. The License field has a value of Unknown but this should probably be MIT (?)

[TimAllen]

Replying to thijs:

1. trunk/twisted/scripts/tap2rpm.py needs a copyright header.

I'm not sure it's entirely kosher for me to add a copyright header talking about Twisted Matrix Laboratories when I don't belong to or represent that organisation, but it's added.

1. I would also get rid of line 4 and open a ticket for it

I believe the original sentiment was to allow packaged services to control their logging configuration, but from ticket #3534 it looks like the Preferred Method is to configure logging from within the .tac file, rather than with twistd command-line arguments. I'll delete the comment and not open a ticket (since #3534 is already closed).

3. The License field has a value of Unknown but this should probably be MIT (?)

I don't think we should assume that code based on Twisted is automatically under the Twisted licence; for example, my employer has internal network services that use Twisted, deployed via tap2rpm, but they're definitely not MIT licensed.

[thijs]

Thanks Tim, your new patch looks good. I noticed that the documentation could probably use an update as well:

• In trunk/doc/core/man/tap2rpm.1:
  • The "REPORTING BUGS" section lists a link to  http://twistedmatrix.com/bugs but I'd rewrite it to say "To report a bug, visit http://twistedmatrix.com/trac/wiki/TwistedDevelopment for more information". That wiki page describes how to report a new ticket, and the old bugs page links to the main trac ticket report that only lists ticket, and doesn't tell you how to actually file a ticket.
  • The copyright date in that man page is outdated
• The section for tap2rpm in trunk/doc/core/howto/basics.xhtml is very brief, if you can add any new useful information there then that would be great
• the same applies to trunk/doc/core/howto/tutorial/configuration.xhtml, if you have a new information that can be added there then that would be very welcome

Thanks!

[TimAllen]

Replying to thijs:

trunk/doc/core/man/tap2rpm.1:

I've updated the "REPORTING BUGS" URL to point to the #FilingTickets anchor on the page you suggested, since it seemed to be the most appropriate (most people who have just noticed a problem aren't likely to have patches and unit-tests handy, which is what that page mostly talks about).

I've also removed references to "unsigned packages", since tap2rpm doesn't support signing packages anyway (those were inherited from tap2deb in the Before Time).

trunk/doc/core/howto/basics.xhtml

The documenation used to be "tap2deb does all these awesome things, oh yeah and there's tap2rpmtoo". I reworked that section to talk about both tools, since they pretty much both do the same job in the same way. I also deleted the advertisement for Twisted-based server packages being available in Debian, since I didn't think that was relevant to a 'what is tap2deb' section.

trunk/doc/core/howto/tutorial/configuration.xhtml

The listed tap2rpm invocation was approximately right, so I just cleaned it up. There's not a lot more to add, really - tap2rpm isn't a very complicated program.

[thijs]

I'm going to merge this, it had a thorough review and shouldn't block the other tickets that came out of this ticket.

[thijs]

(In [27817]) Apply 3292-tap2rpm-fix.patch: Make tap2rpm work with modern versions of RPM and add a basic test suite for this tool.

Author: TimAllen Reviewer: exarkun, glyph, thijs Fixes: #3292

[exarkun]

(In [27829]) Revert r27817 - test suite regression on Windows and Fedora

Reopens: #3292

New tests fail when is not available.

also fails:

Traceback (most recent call last):
Failure: twisted.trial.unittest.FailTest: not equal:
a = {'DESCRIPTION': ['Automatically created by tap2rpm'],
 'NAME': ['twisted-frenchtoast'],
 'RELEASE': ['1'],
 'SUMMARY': ['A TCP server for frenchtoast'],
 'VERSION': ['1.0']}
b = {'DESCRIPTION': ['Automatically created by tap2deb'],
 'NAME': ['twisted-frenchtoast'],
 'RELEASE': ['1'],
 'SUMMARY': ['A TCP server for frenchtoast'],
 'VERSION': ['1.0']}

The tests also seem to generate a large amount of noise on stdout.

[exarkun]

New tests fail when rpmbuild is not available.

twisted.scripts.test.test_tap2rpm.TestTap2RPM.test_basicOperation also fails:

[thijs]

Ah damn, sorry about that, and thanks for spotting and reverting that exarkun. I'll put the patch in a branch when I get time so TimAllen can test and work against that.

[TimAllen]

The test_basicOperation failure is because somebody snuck in and fixed #4083 after I'd submitted this patch for review and but before Thijs merged it. I've rebased my changes onto today's trunk and I'll fix that up.

I'm a bit confused about the rpmbuild-requiring problem; I recall manually testing that, but I suppose I might have missed something. I'll have another look.

The 'lots of noise on stdout' thing is ticket #4085.

[TimAllen]

It turns out that the reason these tests fail when rpmbuild is not available is because of the code I wrote to skip these tests when rpmbuild is not available. In particular, when you call reactor.spawnProcess() and ask for a non-existent command, POSIX gives you back exit code 127, while Win32 gives you:

exceptions.OSError: (2, 'CreateProcess', 'The system cannot find the file specified.')

Filed as #4184. In the meantime, guess I'll need an errback.

[TimAllen]

Here's a new version of the patch:

• Updates tests to check for "tap2rpm" now that #4083 is fixed.
• Add an errback that treats OSError as "rpmbuild cannot be found" (the other reasons OSError might be raised are similarly deadly to testing).

[TimAllen]

Aw crud, I forgot the news files AGAIN.

[TimAllen]

Make tap2rpm work with modern Twisted, attempt 7 (now with news file!)

[TimAllen]

OK, here's a new version of the patch with news file.

[exarkun]

(In [27856]) Branching to 'modern-tap2rpm-3292'

[exarkun]

(In [27857]) Apply TimAllen's 3292-tap2rpm-fix.patch

refs #3292

[exarkun]

(In [27858]) Apply the rest

refs #3292

[exarkun]

Still some Windows issues, looks related to finding rpmbuild still:  http://buildbot.twistedmatrix.com/builders/winxp32-py2.6-select/builds/54/steps/select/logs/problems

twisted.python.procutils.which might be another approach to take on detecting the availability of this.

Patch is in the branch mentioned in the ticket description; please make further patches against that. Thanks!

[exarkun]

Also, when the tests do run and pass, they still produce a lot of junk (presumably rpmbuild output) mixed in with trial's output. It'd be good to eliminate this.

[TimAllen]

Looking more closely at the Win32 tracebacks, it looks like the CreateProcess exception is being raised synchronously, rather than as part of the Deferred errback chain. Rather than wrap getProcessOutputAndValue with even more sanitising, I've switched to t.p.procutils.which as you suggest, which makes things much, much simpler.

I agree that the test spew a lot more to stdout than they really should. As I mentioned, I filed #4085 to keep track of that issue, so that this ticket could be focussed on doing the minimum possible to get tap2rpm working again. If clean test output is a precondition of getting this branch merged, I can do that - I just want to get the first steps merged so I can go on to fixing other stuff.

[radix]

Something is wrong with temp directory locations. I'm getting this when I run tap2rpm:

<a bunch of junk>
Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/radix/rpmbuild/BUILDROOT/twisted-emailserver-1.0-1.i386
Wrote: /home/radix/rpmbuild/SRPMS/twisted-emailserver-1.0-1.src.rpm
Wrote: /home/radix/rpmbuild/RPMS/noarch/twisted-emailserver-1.0-1.noarch.rpm
Executing(%clean): /bin/sh -e /var/tmp/rpm-tmp.csdPHV
+ umask 022
+ cd /home/radix/rpmbuild/BUILD
+ cd twisted-emailserver-1.0
+ [ ! -z /home/radix/rpmbuild/BUILDROOT/twisted-emailserver-1.0-1.i386 -a /home/radix/rpmbuild/BUILDROOT/twisted-emailserver-1.0-1.i386 != / ]
+ rm -rf /home/radix/rpmbuild/BUILDROOT/twisted-emailserver-1.0-1.i386
+ exit 0
Done with build...
======================================================================
Traceback (most recent call last):
  File "/home/radix/Projects/Twisted/trunk/bin/tap2rpm", line 22, in <module>
    tap2rpm.run()
  File "/home/radix/Projects/Twisted/trunk/twisted/scripts/tap2rpm.py", line 265, in run
    rpm_path = glob.glob(os.path.join(tmp_dir, 'RPMS', 'noarch', '*'))[0]
IndexError: list index out of range

Notice that it's writing the RPM file to /home/radix/rpmbuild, but I did some debugging and found that tmp_dir (which is against the Twisted coding standard, btw) is actually something in /var/tmp.

Note that I'm running this on an Ubuntu 9.10 system with RPM version 4.7.0-9.

[TimAllen]

That traceback looks like ticket #4088. Does it work if you run tap2rpm from the same directory as the .tap file?

Yes, there's a lot of very terrible things about tap2rpm that need cleaning up, but I just want to get it working first. :)

[TimAllen]

No response from radix, I'm going to assume that his problem really was #4088 and put this ticket back up for review, in the hope that somebody else gets it working and/or can reproduce the problem that radix bumped into (if it's not #4088).

[exarkun]

I tried this too and got the same exception. I ran tap2rpm in the same directory as the .tap file. Inspecting a bit further, I see that tap2rpm is trying to find the rpms in /var/tmp/tap2rpm-20347-444978795 (that's what tmp_dir is set to on the failing line) but they end up beneath ~/rpmbuild.

I'm on Ubuntu 9.10 fwiw.

[TimAllen]

Thanks.

I can't reproduce the problem on any OS I have ready to-hand (Fedora 6 with RPM 4.4, CentOS 4 with RPM 4.3), but it does seem that tap2rpm goes to some lengths to force RPM to build its output in /var/tmp, while Ubuntu's RPM seems to override somehow and build things in ~/rpmbuild.

It looks like tap2rpm is doing unspeakable horrors to the RPM configuration; maybe it's misconfiguring RPM in the process. I'll see what I can do.

[TimAllen]

This might help the tests to pass on Ubuntu.

[TimAllen]

Jerub in #twisted suggested a better way to set the %_topdir RPM macro (which should determine where rpmbuild tries to build the output), so the attached patch (against branches/modern-tap2rpm-3292) uses that method.

Does that work better?

Also, radix's screenshot suggests that he was manually testing the code - people who can reproduce this issue, please tell me if it also causes the test suite to fail.

[Screwtape]

I've tried this approach on my Ubuntu installation at home, and now tests fail with this error:

Failure: twisted.internet.utils._UnexpectedErrorOutput: got stderr:
'error: cannot open Name index using db3 - No such file or directory (2)\n'

Turns out, RPM on Ubuntu whinges whenever you run it if it doesn't have a database to read ( 1,  2). The workaround is to run "rpm -qa" once as root, then all the tests should pass. Is that an acceptable workaround, or should I find some other way of running rpm that's less picky about stderr?

[exarkun]

You can pass errortoo=True to getProcessOutput and it will mix stderr in with stdout. I think this is a good idea, or at least better than letting the test fail on systems where the functionality is working aside from this warning.

[thijs]

Moving last patch out of review.

[Screwtape]

I had a look at the various options this evening, and frankly none of them are terribly appealing:

• .getProcessOutput with errortoo=False explodes with Debian's misconfigured RPM installation.
• .getProcessOutput with errortoo=True makes it too easy to ignore helpful error messages from RPM along with this bogus warning. Also, the exact output I receive depends on the order in which the reactor happened to check the file stdout/stderr descriptors. Also also, if RPM ever printed more than a bufferful of output, it's quite likely the streams would be interleaved.
• .getProcessOutputAndValue would let me check rpm's exit code but doesn't let me separate the stdout/stderr streams, leading to the same problems as above.
• Rolling my own IProcessProtocol and .spawnProcess() wrapper is possible, but also sounds like a lot of work for something that should be fairly simple.
• Making this bug blocked by #3997 and #280 sounds like even more work.

Given that this is an upstream bug that only affects the test suite, only on platforms where this functionality will be rarely-if-ever used, and it has a very simple work-around for people like buildbot admins who just need to get things working ("sudo rpm -qa > /dev/null"), I'd like to leave this code as-is, and leave the corner-case of misconfigured-RPM-on-Debian as a bug that waits on #3997.

I'm putting this back in the review queue, hoping that the branch will be merged, but I'd also be happy to hear anyone's arguments that the misconfigured-RPM-on-Debian problem should be addressed before merging.

[khorn]

I haven't even looked at the code, so bear with me, but would it be possible to spit out a message that refers to the sudo rpm -qa > null workaround (or maybe just to this ticket) when the misconfigured-RPM-on-Debian problem is encountered?

[exarkun]

Aside from being a bit ugly and confusing, what problems are introduced by mixing stdout and stderr? If the exit code is sufficient to tell if the operation succeeded or failed, isn't it basically okay to let stdout and stderr mix together, since you'll ignore them in almost all cases anyway?

As far as requiring buildbot admins to things, it's a challenge to spur them into action, so if we can avoid needing to this, all the better. I'd also be good to avoid introducing a test case that's going to fail on subtly (harmlessly?) misconfigured systems.

I agree that blocking on #280 or #3997 should be avoided if possible, though.

[Screwtape]

Yes, I suppose there should be something that spits out a helpful error message if this problem is detected. I'll do that.

Exarkun, the test harness is using rpm's "query" mode to verify that the metadata we told tap2rpm to write into the package has actually been written (things like package-name, version, maintainer, etc.). rpm writes this information to stdout, so I'd rather not have to pick bits of stderr out of it before I can parse it.

[Screwtape]

Fixes for running RPM tests on Debian and Ubuntu.

[Screwtape]

Fixes for running RPM tests on Debian and Ubuntu.

[Screwtape]

OK, here's a patch against branches/modern-tap2rpm-3292 that makes life a little better for RPM on Ubuntu:

• As before, we ask RPM to build packages in /var/tmp the polite way, instead of trying to build a working RPM config file from a randomly-chosen subset of existing config files.
• When .getProcessOutput() recieves stderr data and raises an exception that might possibly be a symptom of the misconfigured-RPM-on-Debian problem, our new errback raises SkipTest with a helpful error message instead.

Unfortunately rpm seems to call write("error: ") followed by write(errorText), so the exception .getProcessOutput() raises sometimes has the complete error text, and sometimes just has "error: " which it makes it hard to tell exactly which error was encountered. Fixing this false-positive problem *does* block on #3997.

Another wrinkle in the new errback is that .getProcessOutput raises its "stderr data" exception immediately, rather than when the process ends - leading to bogus extra errors even if SkipTest was raised. Thus, we have to mess around to make sure the exceptions are raised after the process exits.

[exarkun]

(In [28062]) Apply modern-tap2rpm-3292-2.patch

refs #3292

[exarkun]

These tests still write a huge amount of stuff to stdout. This needs to not happen. Sorry I wasn't more emphatic on this point earlier. I agree that small changes are good, and incremental progress towards an ideal tap2rpm is good, but we really can't add 5 pages of junk output here.

Skipping any other review feedback for now so that we can decide what to do about just this one point without getting side-tracked by other things again. :)

[TimAllen]

Make tap2rpm quieter by running it with subprocess.Popen

[TimAllen]

I've attached a patch that runs rpmbuild with subprocess.Popen rather than os.system, which makes the test suite a lot quieter (although not silent). So that tap2rpm is still useful outside the test suite, the output is cached and only displayed if rpmbuild exited with a non-zero status.

There's no unit-test for this behaviour; my first thought was to swizzle sys.stdout with a StringIO and check that nothing was written to it after the test, but since the code that prints to stdout is in a subprocess that trick doesn't work and I couldn't think of another.

In an ideal world, t.s.tap2rpm.run() would be broken up into testable pieces, and the unit-tests wouldn't have to run the user-interface print-to-stdout code at all, but that's a problem for another ticket, I think.

[thijs]

(In [28116]) Apply modern-tap2rpm-3292-3.patch, refs #3292

[jml]

I've reviewed this ticket, and think that the code here is pretty good and almost ready to land.

There are some cosmetic issues, minor grammar things and what-not. I've fixed those.

The tests were spewing output during the test run. That sucked, so I fixed that too by adding a --quiet option. Because that needs an actual change, I need to ask someone else to review it.

Thanks very much Tim for your persistence and hard work in getting this patch in. The end is nigh!

[TimAllen]

Looks good to me.

I was thinking of deleting those 'Writing "%s"...' messages in a follow-up ticket, but I think the --quiet option is overall a better choice.

+1 for merging, from me.

[jml]

(In [28357]) Merge modern-tap2rpm-3292

• Author: TimAllen, jml
• Reviewer: glyph, exarkun
• Fixed #3292

Revive the long-dead tap2rpm.

[jml]

(In [28364]) Revert r28357. Reopens #3292.

The RPM tests fail on Fedora buildbot.

[TimAllen]

It's not obvious to me why the tests are failing on Fedora64. The  buildbot output doesn't show the rpmbuild output as it should, and the  build errors discovered (presumably) interactively by therve make it sound like the generated .spec file is empty or corrupt in some way, but it's not at all clear why.

Possible avenues of inquiry:

• Temporarily back out the 'make quiet' changes in this branch, including the --quiet flag, and the code that hides the rpmbuild output if it exits with a safe-looking zero exit code.
• Add -v to the rpmbuild invocation (or possibly -vv).

[TimAllen]

A quick hack for maximum debugging output.

[TimAllen]

I've attached a very quick hack that turns the debug spew up to 11 to help diagnose these test failures on Fedora64. I'd appreciate it if somebody could temporarily apply this patch to the branch (or a branch of the branch) and push it through the Fedora64 build-bot, then link me to the resulting build logs.

[TimAllen]

Try making the RPM tarball in python, instead of shelling out to tar.

[TimAllen]

Getting desperate now, let's try running rpmbuild inside strace.

[TimAllen]

The  output of the strace-enabled build is telling.

The way RPM works, a package is built from either an SRPM (source RPM), or a pristine source tarball and a .spec file. Making sure the .spec file and the tarball are in the right places and have the right cross-references can be tricky, so rpmbuild contains a special mode where you give it a source tarball that contains the .spec and it figures out the path references itself. This last mode is what tap2rpm uses.

How does rpmbuild figure out which file in the tarball is the .spec file? So far as I can tell from reading the strace output (naturally, it's not documented anywhere that I can find), the plan goes something like this:

1. Extract from the tarball the file named Specfile, dump the output into a temporary file.
2. If Specfile was found, its contents are the name of the .spec file to use.
3. If it's not there, dump a list of files in the tarball matching *.spec to a temporary file.
4. Extract from the tarball the file whose name is the exact contents of that temporary file.

Currently, tarballs generated by tap2rpm do not contain a Specfile, so we fall back to the second rule. On my workstation running Fedora 6, strace tells me that it searches for files matching *.spec with the following command:

gunzip < /tmp/tmpNOUXa_/twisted-frenchtoast-1.0.tar.gz | tar xOvf - --wildcards \\*.spec 2>&1 > /tmp/tmpNOUXa_/SPECS/rpm-spec.zNCc7L

This produces an output file whose contents are:

twisted-frenchtoast-1.0/twisted-frenchtoast.spec

which is exactly correct.

On the fedora64-py2.4-reactors buildbot (apparently running the older Fedora 5), I don't have enough strace options to see the equivalent command-line, but I can see that the output file contains:

tar: Pattern matching characters used in file names. Please,
tar: use --wildcards to enable pattern matching, or --no-wildcards to
tar: suppress this warning.
tar: *.spec: Not found in archive
tar: Error exit delayed from previous errors

It seems  Fedora 5 came with GNU Tar 1.15, and the warning in question was  added in version 1.22, so this buildbot has been misconfigured.

Short of replacing or reconfiguring the buildbot, I guess the only remaining option is to teach tap2rpm to add a Specfile that tells rpmbuild what .spec file to use.

[TimAllen]

Replying to TimAllen:

It seems  Fedora 5 came with GNU Tar 1.15, and the warning in question was  added in version 1.22, so this buildbot has been misconfigured.

My apologies - it seems this  really is a bug in Fedora 5.

The rabbit-hole goes deeper: I had thought that the magic Specfile file contained a pointer to the actual .spec file, but  this RPM bug suggests it should contain the actual spec data. When I do that, rpmbuild on my workstation can't find the spec at all. Looking at  the RPM source-code that implements the search for the specfile, I'm afraid my C is too rusty to figure out exactly what it expects, but it may not matter: Every  newer  version  of  RPM has refactored that messy code into a for-loop with no break, so even I can see that Specfile will be accidentally ignored in modern RPM versions.

The only remaining avenue of inquiry I can see is to figure out how to build an RPM from a source tarball and an external .spec file.

[therve]

Great investigation Tim! I don't think we should try to workaround a bug in a old Fedora version. Can you find a way to skip the tests instead, checking the tar version or something?

[jml]

I agree with therve. However, for the mean time, I'm backing this patch out of the 10.0 branch.

[TimAllen]

It's not so much the version of tar as a mismatch between the versions of tar and rpm, and I'm not terribly keen to work out a matrix of which tar versions work with which rpm versions. Also, the relative bugginess of this RPM feature is a hint that RedHat don't actually use it very much, and it would be a wise move to return to better-tested ground.

[TimAllen]

Here is a new patch against the current branch.

1. It removes all the hacky verbose-debugging goo added in previous comments. Previously there were hardcoded references to /var/tmp in various places; therve neutered one of them while debugging, so I've now removed the others.
2. Previously, the MyOptions class had empty defaults for most variables, and there was a bunch of logic in run() that populated them. This logic has been moved into the MyOptions class where it belongs. Tests of the MyOptions class are added.
3. Previously, the code built an RPM by creating a bunch of files in a directory, making a tarball, the handing the tarball to rpmbuild -ta. Now we create the source tarball and the spec file separately, and hand them to rpmbuild -ba. I suspect this is how Red Hat builds their own RPMs, and hence is likely to be much more reliable.

As part of the above changes, I've moved the 'create RPM source material' code into its own function called setupBuildFiles(), and replaced all the uses of vars() with explicit parameters for maintainability. As a side-effect, I suspect I may have fixed #4085 and #4088.

I'm putting this up for code-review, but in particular, I'd appreciate it if somebody could commit this patch to the branch and see if it works on the  fedora64-py2.4-reactors buildbot.

[therve]

(In [28560]) Apply patch

Refs #3292

[therve]

(In [28572]) Merge modern-tap2rpm-3292

Authors: TimAllen, jml Reviewers: glyph, exarkun, therve Fixes #3292 Fixes #4085

Revive the long-dead tap2rpm. This remerge fixes failure with RPM/Tar versions under Fedora 5.