Spec URL: http://www.guthrie.info/RPMS/fc6/dxpc.spec SRPM URL: http://www.guthrie.info/RPMS/fc6/dxpc-3.9.0-3.fc6.src.rpm Description: Hello, I just finished repackaging the dxpc package from fc5 with the latest version of dxpc. This is the first time that I have submitted a package for review. I am hoping to get it reviewed so that I can get it into fc6. Dxpc is a differential X protocol compressor. dxpc is an X protocol compressor designed to improve the speed of X11 applications run over low-bandwidth links (such as dialup PPP connections or ADSL). This RPM has been updated to the most recent version of the code. (3.9.0) Please note that I have signed the SRPM file with a gpg key. You can verify it with the key here: http://www.guthrie.info/RPMS/GUTHRIE-GPG-KEY. (I didn't know if this was the right thing to do, but I figured it wouldn't hurt.)
I should note that the release tag for this RPM is not 1.fc6 because I took a patch for Makefile.in that was part of the previous RPM for fc5.
I suggest removing the [ "%{buildroot}" != "/" ] part from %install and %clean. No one is using / for buildroot any more.
Good: - rpmlint checks return: on dxpc-3.9.0-3.fc6.src.rpm E: dxpc unknown-key GPG#be3aac96 W: dxpc macro-in-%changelog buildroot W: dxpc mixed-use-of-spaces-and-tabs (spaces: line 2, tab: line 7) first one can be ignored. second one is caused by not prefixing %{buildroot} with a % in 3.9.0-2. Third one is obvious... - no output from rpmlint for the binary rpm - package meets naming guidelines, except that release tag could have started from 0 or 1; the presence of a patch taken from a previous version does not really justify starting from 3. This is not a blocker. - package meets packaging guidelines, except for the presence of [ "%{buildroot}" != "/" ] - license is BSD, corresponding to the one provided in the sources as README - spec file legible, in am. english - source is last version, matches upstream, sha1sum 48acc713a8d3386d8f554fdba8b93dd8cc0e28c4 dxpc-3.9.0.tgz - package compiles on devel (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates, does not own foreign files/dirs - no duplicate files - permissions ok - MUSTFIX: removing the [ "%{buildroot}" != "/" ] from %install and %clean - macro use consistent; "make install" from sources does not work correctly (build fails, so usage of %makeinstall is justified (unless a patch is written for Makefile.in)) - code, not content - no need for -docs; README.mingw could be ditched, it is not relevant for Fedora; TODO also does not contain anything useful - nothing in %doc affects runtime - no need for .desktop file, no .la, static files, or .pc SHOULD - builds succesfully in mock,FC6/x86_64 - installs cleanly, runs OK MUSTFIX: - removing [ "%{buildroot}" != "/" ] Suggested fix: - not packaging README.mingw Since you need a sponsor, I cannot formaly approve your package, but once you find a sponsor, please point him to this review. Please follow the instructions from http://fedoraproject.org/wiki/Extras/Contributors in order to get sponsored.
Ups.. sorry Michael, I have not noticed you have already taken this request :(
> No one is using / for buildroot any more. Because it is impossible. :) Apart from that you would find it very difficult to find somebody who has ever before killed a machine with rpmbuild and "--buildroot /".
In addition to the findings from comment 3, a bit of clean-up could be applied to the spec file. All non-blockers, though: > BuildRequires: lzo-devel >= 1.08 lzo is >= 1.08 in Fedora, at least in Fedora Extras >= 3. > %if "%{?fedora}" > "4" Fedora <= 4 has reached end-of-life. > %install > [ "%{buildroot}" != "/" ] && rm -rf %{buildroot} > %makeinstall Instead of %makeinstall, prefer the standard make DESTDIR="%{buildroot}" install whenever it works. These can all be fixed in CVS, however, so feel free to continue with the process at step 10: http://fedoraproject.org/wiki/Extras/Contributors
Well, at least you wouldn't catch me doing that anyway. ;-) I've seen that test in several other RPMs, and I figured it wouldn't hurt to protect against someone doing something silly. But I can also understand we're trying make the spec files as clean as possible. I have reposted the spec file and the source rpm at the URLs above. I have removed the test for %{buildroot} as requested. I have cleaned up the warnings from rpmlint for the source rpm. (So now you should only get the warning about the unknown key.) I have also removed README.mingw from the documentation.
(The first sentence of the last comment applied to comment #5, BTW.) I should note that the Makefile.in makes absolutely no use of the variable DESTDIR at this moment. Looking at the Makefile, that shouldn't be too difficult to fix. I'll leave those fixes for CVS, like you said. Thanks again for your help.
WRT comment #6: I see no point in dropping support for FC<=4 as long as it comes with no extra cost. We do not build any more in FE for those distros, but someone using them -- or a derivative -- might just grab the src.rpm and recompile it. I for one am forced from time to time to build for Centos 3 & 4 (take for instance nedit.. the one in Centos 3 does not have multitab editing, an extremely useful feature for my colleagues). I find easier to just start from a validated src.rpm which already includes the needed bits than do the all the backporting myself... even if we are speaking of just replacing the names of the xorg libs. And yes, I do know the risks involved by using older distros as well as those coming from backporting. wrt comment #7 Whenever modifying the spec file, please increase the release tag and add a relevant entry in %changelog.
I think there's a problem on 64 bit. Here's what happens when I just tested this on x86_64: (this is a runtime error) dxpc proxy running in CLIENT mode using image compression level 9 connected to server proxy ready [now connect...] dxpc: EncodeBuffer.C:95: void EncodeBuffer::encodeValue(unsigned int, unsigned int, unsigned int): Assertion `numBits <= (sizeof(unsigned) * 8)' failed.
I will probably have to pass this bug upstream for the time being and add an excludearch tag to the spec file.
That could be due to a rewrite of the EncodeBuffer/DecodeBuffer code in 3.9.0. Interestingly, the Debian folk offers this dxpc for AMD64 for several months without any patch in that area of the code: http://packages.debian.org/unstable/x11/dxpc In the source I see a 32-bit bit-shifting en-/decoding working with "unsigned int" most of the time in many places, but sometimes only with simple type-specifiers. Maybe there's an oversight somewhere. Without an x86_64 test machine I won't even start careful proof-reading, however. Also, the upstream authors are encouraged to use full type-specifiers wherever possible, e.g. "unsigned int X" instead of "unsigned X" and "sizeof(unsigned int)" instead "sizeof(unsigned)".
Good news! I just received this email from upstream: > Date: Thu, 01 Feb 2007 22:58:35 -0700 (Fri, 00:58 EST) > From: Kevin Vigor <kevin> > To: John T. Guthrie III <guthrie> > Subject: Re: Problem with dxpc on 64-bit machines? > John, > I have just put together a probable fix for this problem; lacking a > 64-bit system to test it on, I am awaiting feedback from the user who > first reported it. If you would like to try the proposed fix yourself > you can grab dxpc 3.9.1 beta 1 from http://www.vigor.nu/dxpc-3.9.1b1.tgz. > If you can't or don't want to test this version I will let you know > the results of testing as soon as I know them. > Thanks, > Kevin I have asked Kevin for a patch file as well. At this point, I could go ahead and get the new version of the software and re-package it, but I don't know how that would affect the current packaging process. Or we could just do a diff. Any thoughts?
The diff against 3.9.0 is just 4629 in size. You could apply it *or* simply build a dxpc-3.9.1-0.1.b1%{?dist} package in accordance with versioning scheme for pre-releases.
I've re-packaged and renamed the SRPM with the new versio of the source code. The SRPM is here: http://www.guthrie.info/RPMS/fc6/dxpc-3.9.1-0.1.b1.fc6.src.rpm The new spec file is here: http://www.guthrie.info/RPMS/fc6/dxpc.spec The version is now 3.9.1. The release is now 0.1.b1%{?dist}. I have updated the %changelog section thi time as well. ;-) Also, I have been trying to upload the SRPM into CVS, and I keep getting the following output: euler_1125% ./common/cvs-import.sh ~/rpm_build/SRPMS/dxpc-3.9.1-0.1.b1.fc6.src.rpm Checking out the modules file... Module 'dxpc' already exists... Checking out module: 'dxpc' Unpacking source package: dxpc-3.9.1-0.1.b1.fc6.src.rpm... R dead.package A dxpc-3.9.0-dxpcssh.patch A dxpc-3.9.0-mandir.patch L dxpc-3.9.1b1.tgz A dxpc.spec make: *** No rule to make target `upload'. Stop. ERROR: Uploading the source tarballs failed! Can someone point me in the right direction to fix this? Do I need to cat the Makefile.common onto the end of the Makefile in the common directory that I just checked out? Thanks.
Nope. Try again. It should work now. dxpc package is a special case in that it had been marked as a "dead.package" in CVS some time before FC6. Such packages must be resurrected before cvs-import.sh will work again. FYI: After successful import to "devel", the FC-6 branch for dxpc doesn't exist and would need to be requested in the Wiki: http://fedoraproject.org/wiki/Extras/CVSSyncNeeded
Thanks. I was able to get the import to work correctly after you did that. (There were some scary-looking tracebacks in there as well.) I just edited the page you mentioned with the request for the new braches. Since there is a dxpc package in FC5, I asked for both FC-5 and FC-6 branches. Also, upstream sent word that 64-bit testing has gone well. Neal(from comment #10), would you be able to test the new software out as well? Thanks.
* FC-5 branch exists already. You can update it yourself within CVS. ( http://fedoraproject.org/wiki/Extras/UsingCvsFaq ) * The tracebacks are due to CVS ACL and mail-notification changes from Jan 31st. Packages, which are still orphaned, apparently give such a traceback. * Editing owners/owners.list in CVS is no longer possible since Jan 31st: - owners.list and owners.epel.list are now locked down. To request changes, please send mail to cvsadmin-members. (This may be replaced with the wiki or the ticketing system really fast.)
Seems to be working OK now on fc6 x86_64. Thanks!
I have one final question. I have just made a patch to the Makefile.in so that we can use "make DESTDIR=%{buildroot}" instead of %makeinstall. Since this is a brand new version anyway, do we want to increment the release, or just leave it at 0.1.b1? (I guess it would increment to 0.2.b1.) Or is this up to my discretion? Other than that, I'm ready to check things into CVS.
A different way to phrase my previous question is do I need to increment the release if I haven't tagged anything in CVS yet?
I would say that although in this case practically there would be no change seen by the final users, any modification to the spec file -- and above all when you modify the source -- should be reflected in the release field (and a proper comment added to changelog). And yes, 3.9.0-0.2.b1 would be correct (see last line in the second set of examples -- incremented Fedora %{name}-%{version}-%{release} -- at http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-d97a3f40b6dd9d2288206ac9bd8f1bf9b791b22a)
And cvs-import.sh has also tagged the imported files (with a wrong .fc6 tag, btw, because it was set in the src.rpm).
That's odd. The .spec file uses %{?dist}, not an actual distribution. Should that be unset when I make a tag in the devel branch?
This is what the import logged: > Log Message: > auto-import dxpc-3.9.1-0.1.b1.fc6 on branch devel from > dxpc-3.9.1-0.1.b1.fc6.src.rpm And "cvs log dxpc.spec" agrees with that. The tag dxpc-3_9_1-0_1_b1_fc6 is on the files in "devel" and cannot be reused for the FC-6 branch. This is because the src.rpm was built with "--define dist 6" instead of leaving %dist undefined. When the src.rpm is queried, ".fc6" is part of %release already, and for files in "devel". It is a problem that has come up recently. Most people import src.rpms where they use %{?dist}, but where %dist is undefined when the src.rpm is built. The way out is to increase "Release" in every branch and run "make tag" to create fresh tags in every branch. Not a big issue, but unpleasant trouble just because of %{?dist}. :)
Okay, I tagged the latest version in FC-6. According to the job.log files, it built correctly for all three architectures. Unless there is anything else that needs to be done, I'll close this bug. On a slightly different note, if I try typing "make build" in the devel directory, then I get the error message: dxpc.spec not tagged with tag dxpc-3_9_1-0_2_b1_fc7 make: *** [build] Error 1 So then if I type make tag, I get the following: cvs tag -c dxpc-3_9_1-0_2_b1_fc7 ERROR: Tag dxpc-3_9_1-0_2_b1_fc7 has been already created. The following tags have been created so far dxpc-3_8_2-2:devel:mschwendt:1112826930 dxpc-3_8_2-3_fc5:devel:rdieter:1138638471 dxpc-3_8_2-3_fc5_1:devel:rdieter:1139580877 dxpc-3_8_2-3_fc5_2:devel:rdieter:1141231178 dxpc-3_9_0-1_fc6:devel:jwrdegoede:1154277450 dxpc-3_9_1-0_1_b1_fc6:devel:guthrie:1170455257 dxpc-3_9_1-0_2_b1_fc7:devel:guthrie:1170694597 dxpc-3_9_1-0_2_b1_fc6:FC-6:guthrie:1170696651 cvs tag: Pre-tag check failed cvs [tag aborted]: correct the above errors first! make: *** [tag] Error 1 It looks like there is a dxpc-3_9_1-0_2_b1_fc7 tag in devel. Am I just not supposed to do builds in the devel directory? Also, what is the easiest way to find out what tags have already been created? Thanks.
The succeeded in devel as well, so I'm closing this bug.
Package Change Request ====================== Package Name: dxpc New Branches: F-11 Owners: guthrie
Never mind. I was trying to check the F-11 branch out incorrectly. I figured it out now.