[Bug 174063] Review Request: cssed - css editor and validator

[bugzilla at redhat.com]

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug report.

Summary: Review Request: cssed - css editor and validator


https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=174063





------- Additional Comments From fedora at iagorubio.com  2005-11-28 06:41 EST -------
Thanks for your review, Aurelien.

>> Specfile should be in the format %{name}.spec

I don't completely understand this. It's already in this format, isn't it ?

cssed.spec so %{name}.spec or, may be I'm missing something ?

>>Line 1: dont use define prefix, it is already setup for you (as _prefix)
if really needed.

Fixed.

>>Line s 2-3: don't define version and release on top of the spec file.

Fixed

>>Line 12: use http://dl.sourceforge.net/cssed/cssed-%{version}.tar.gz

Fixed

>>Lines 15 & 26: Don't use buildarch i386

Fixed.

>>Line 27:  -devel package should have "Requires: %{name}-%{version}"

Fixed.

>>Line 34: you only need "%setup -q",

Fixed.

>>Line 37:  export CFLAGS is useless, it will be done by %configure

Fixed.

>> Line 38: --prefix and --mandir are already defined by %configure

Fixed

>> Line 39:  missing SMP flags.

Fixed

>> Line 42:       the check is useless and misleading

Fixed

>> Line 46:       why remove the man file ?

Fixed.

>> Lines 47-67:   why remove the desktop file

Fixed

>> Line 60:  you don't need to install the desktop file in the buildroot

Fixed

>> Line 70:  don't call make install twice

Fixed

>> Lines 71-73: Just remove this dir since you add the doc with a %doc tag in
the %files list 

Fixed

>> Lines 74-78:   this directory should be moved in %{_includedir}/cssed, where
include files are.

This will need upstream chages to the cssed.pc.in file. Not a problem but I must
push a new package to sourceforge, so I prefer to wait for the other changes to
be reviewed, because It could be needed other changes to the package and I
prefer to commit them all at the same time.
PENDING

>> Line 81:  for %clean, same thing as for %install. Just stick to rm -rf
%{buildroot}

Fixed

>> Lines 87-119:  just use %{_datadir}/cssed. It's recursive.

Is this mandatory ? I prefer to install individual files to avoid to install
bogus files. If a file slips into the package's directory, I prefer an error
than to install the bogus file by mistake.

ITOH if it's mandatory I have no problem to change. It's only I think the
recursive install is the easy a dirty way :)


>> Lines 121-126: use the %find_lang macro to list the translations

Fixed

>> Lines 132-138: the include files should be in %_includedir/cssed

It's PENDING above waiting for other changes to the package. If no other
problems arise I will push a new package upstream with the cssed.pc.in file
reflecting the new include directory.

>>  Line 139:      you don't need the docs in the -devel package. They are
available in the main package

Fixed


Changes uploaded to the same location:
http://iagorubio.com/fedora/cssed.spec
http://iagorubio.com/fedora/cssed-0.4.0-0.src.rpm

Old spec reacheable at:
http://iagorubio.com/fedora/cssed.spec.bak-1


All reviewed items corrected but:

- Specfile should be in the format %{name}.spec
  PENDING of further comments.

- Lines 74-78:   this directory should be moved in %{_includedir}/cssed
  PENDING to push changes to upstream package.

- Lines 87-119:  just use %{_datadir}/cssed. It's recursive.
  PENDING of further comments.

With the corrected spec file cssed builds fine and rpmlint does not complain.

-- 
Configure bugmail: https://bugzilla.redhat.com/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug, or are watching someone who is.