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.