Bug 225126 - Review Request: dxpc - A Differential X Protocol Compressor
Review Request: dxpc - A Differential X Protocol Compressor
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2007-01-29 00:00 EST by John Guthrie
Modified: 2009-04-23 15:40 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-02-05 15:22:12 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description John Guthrie 2007-01-29 00:00:30 EST
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.)
Comment 1 John Guthrie 2007-01-29 00:09:38 EST
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.
Comment 2 manuel wolfshant 2007-01-29 03:53:52 EST
I suggest removing the [ "%{buildroot}" != "/" ] part from %install and %clean.
No one is using / for buildroot any more.
Comment 3 manuel wolfshant 2007-01-29 05:24:16 EST
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.
Comment 4 manuel wolfshant 2007-01-29 05:25:29 EST
Ups.. sorry Michael, I have not noticed you have already taken this request :(
Comment 5 Michael Schwendt 2007-01-29 06:23:12 EST
> 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 /".
Comment 6 Michael Schwendt 2007-01-29 13:53:15 EST
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
Comment 7 John Guthrie 2007-01-29 14:27:58 EST
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.
Comment 8 John Guthrie 2007-01-29 14:33:03 EST
(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.
Comment 9 manuel wolfshant 2007-01-29 17:25:45 EST
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.
Comment 10 Neal Becker 2007-01-30 20:01:05 EST
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.
Comment 11 John Guthrie 2007-01-31 02:37:03 EST
I will probably have to pass this bug upstream for the time being and add an
excludearch tag to the spec file.
Comment 12 Michael Schwendt 2007-01-31 05:16:55 EST
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)".
Comment 13 John Guthrie 2007-02-02 01:34:11 EST
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@vigor.nu>
> To: John T. Guthrie III <guthrie@counterexample.org>
> 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?
Comment 14 Michael Schwendt 2007-02-02 04:16:03 EST
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. 
Comment 15 John Guthrie 2007-02-02 16:37:01 EST
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.
Comment 16 Michael Schwendt 2007-02-02 17:25:37 EST
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
Comment 17 John Guthrie 2007-02-02 18:50:51 EST
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.
Comment 18 Michael Schwendt 2007-02-03 05:31:44 EST
* 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@fedoraproject.org.
  (This may be replaced with the wiki or the ticketing system really
  fast.)
Comment 19 Neal Becker 2007-02-03 08:06:37 EST
Seems to be working OK now on fc6 x86_64.  Thanks!
Comment 20 John Guthrie 2007-02-04 21:38:00 EST
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.
Comment 21 John Guthrie 2007-02-04 21:58:37 EST
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?
Comment 22 manuel wolfshant 2007-02-05 01:39:01 EST
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)
Comment 23 Michael Schwendt 2007-02-05 04:31:07 EST
And cvs-import.sh has also tagged the imported files (with a wrong
.fc6 tag, btw, because it was set in the src.rpm).
Comment 24 John Guthrie 2007-02-05 11:18:12 EST
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?
Comment 25 Michael Schwendt 2007-02-05 11:37:07 EST
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}. :)
Comment 26 John Guthrie 2007-02-05 14:07:37 EST
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.
Comment 27 John Guthrie 2007-02-05 15:22:12 EST
The succeeded in devel as well, so I'm closing this bug.
Comment 28 John Guthrie 2009-04-23 15:35:02 EDT
Package Change Request
======================
Package Name: dxpc
New Branches: F-11
Owners: guthrie
Comment 29 John Guthrie 2009-04-23 15:40:30 EDT
Never mind.  I was trying to check the F-11 branch out incorrectly.  I figured it out now.

Note You need to log in before you can comment on or make changes to this bug.