Bug 207741 - Review Request: colorsvn - subversion output colorizer
Review Request: colorsvn - subversion output colorizer
Status: CLOSED WONTFIX
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Package Reviews List
:
Depends On:
Blocks: FE-DEADREVIEW
  Show dependency treegraph
 
Reported: 2006-09-22 15:59 EDT by V. Klein
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-10-15 19:12:51 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)

  None (edit)
Description V. Klein 2006-09-22 15:59:16 EDT
Spec URL: http://www.console-colors.de/downloads/colorsvn/colorsvn.spec
SRPM URL: http://www.console-colors.de/downloads/colorsvn/colorsvn-0.3.1-1.src.rpm
Description: colorsvn is the Subversion output colorizer. Colorsvn was extracted from kde-sdk and was extended with build process and configuration.
Comment 1 V. Klein 2006-09-22 16:02:51 EDT
This is my first package, so I am looking for a sponsor.
Comment 2 Francois Aucamp 2006-10-11 05:49:46 EDT
A quick review: (I'm not an official reviewer, but I'm interested in this :-) )

Review items:
* rpmlint output (run rpmlint with -i for more info):
E: colorsvn sourced-script-with-shebang /etc/profile.d/colorsvn-env.sh
E: colorsvn executable-sourced-script /etc/profile.d/colorsvn-env.sh 0755

* package meets naming guidelines
* specfile is properly named, uses macros fairly consistently (see NOTES below)
* package license is GPL
* license field in spec file matches actual license

* license file is NOT included in %doc (see NOTES, below)

* source md5sum matches upstream:
dca88aacc8e7437a9b39e2449dbe678f  colorsvn-0.3.1.tar.gz

* package does NOT build in mock (FC5/i386, FC5/x86_64):

<snip>
checking for svn... no
configure: error: svn is required
error: Bad exit status from /var/tmp/rpm-tmp.11087 (%build)


RPM build errors:
    Bad exit status from /var/tmp/rpm-tmp.11087 (%build)
</snip>

You need to add "subversion" as a BuildRequires, or modify the configure script

* BuildRequires is incomplete (see above comment)

* no %post and %postun sections (no need for them)
* package is not relocatable
* directory/file ownership correct
* no duplicates in %files, %defattr is proper (see NOTES below)
* proper %clean section
* uses $RPM_BUILD_ROOT consistently; macros used well
* no need for seperate -doc or -devel subpackages
* no .desktop file needed (not a GUI app)

* package does not meet Fedora packaging guidelines (due to errors mentioned above)

* code, not content
* no locales
* %docs are not necessary for the proper functioning of the package


NOTES:
* You don't have to explictly list each %doc entry; just seperate them with spaces
* Add COPYING to the %docs
* You can use %{buildroot} instead of $RPM_BUILD_ROOT for full macro consistency
(not actually required, just a suggestion)
* In %files, instead of %{_prefix}/bin/colorsvn, you can use %{_bindir}/colorsvn
(just a suggestion)

By the way, shouldn't this bug also block FE-NEW ? 
I'm adding the block, but please remove it if I am missing something... ;-)
Cheers
Comment 3 Francois Aucamp 2006-10-11 05:57:40 EDT
I forgot to add the following....
* rpm does NOT install properly (dependant on host setup):

Preparing...                ########################################### [100%]
        file /usr/bin/colorsvn from install of colorsvn-0.3.1-1 conflicts with
file from package kdesdk-3.5.4-1.0.fc5.kde

I know you mention the kde-sdk origins in your description, but iou might want
to add a "Conflicts" statement specifying this conflict in your spec file, for
people running KDE.
Comment 4 V. Klein 2006-10-14 07:00:30 EDT
Thanks for your review and suggestions!

I have made some changes on the spec file according to your suggestions. But I
really don't know what to do with the rpmlint error. I think it is not right to
add a shebang line to the script because all another scripts in this directory -
/etc/profile.d - don't have the shebang too. But if I am wrong, so correct me.

Cheers
Comment 5 Kevin Fenzi 2006-10-14 13:30:16 EDT
In reply to comment #3:

Note that extras packages should not conflict with core packages. 
If kdesdk provides /usr/bin/colorsvn (which it does), I fear this package will 
not be acceptable for extras.

What is the diffrence between this version and the one provided by kdesdk?

Comment 6 V. Klein 2006-10-15 04:50:00 EDT
(In reply to comment #5)
> In reply to comment #3:
> 

> What is the diffrence between this version and the one provided by kdesdk?
> 
Separated configuration file, minor changes in code. The intention was to
extract it from kdesdk because colorsvn can be used without the kde stuff.

> Note that extras packages should not conflict with core packages. 
> If kdesdk provides /usr/bin/colorsvn (which it does), I fear this package will 
> not be acceptable for extras.

You have right, both packages are in conflict. This request can be deleted,
don't know how to do it.
Comment 7 Kevin Fenzi 2006-10-15 19:12:51 EDT
Perhaps you could talk with the kde folks upstream and see if they would be 
willing to stop shipping it? If they do, please do resubmit this to extras, or 
reopen this review request. 


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