Bug 174063

I'm sorry, I forgot to mention that it's my first package and I'm seeking for a
sponsor.

Needs work:
* Specfile should be in the format %{name}.spec
  (wiki: PackageReviewGuidelines)
* Spec file :

Line 1:        dont use define prefix, it is already setup for you (as _prefix)
if really needed.
Line s 2-3:    don't define version and release on top of the spec file. Setting
the tag automatically sets the macro.
Line 12:       use http://dl.sourceforge.net/cssed/cssed-%{version}.tar.gz as
the Source tag, to be able to download and check the source directly
Lines 15 & 26: Don't use buildarch i386 (what about x86_64 and ppc ?). Don't use
buildarch: noarch for the -devel rpm also
Line 27:       -devel package should have "Requires: %{name}-%{version}"
Line 34:       minor : cssed-%{version} is already the default dir name, so you
only need "%setup -q", no need for the -n option
Line 37:       export CFLAGS is useless, it will be done by %configure
Line 38:       --prefix and --mandir are already defined by %configure
Line 39:       missing SMP flags. If it doesn't build with it, please add a
comment (wiki: PackagingGuidelines#parallelmake)
Line 42:       the check is useless and misleading, just use rm -rf %{buildroot}
Line 46:       why remove the man file ?
Lines 47-67:   why remove the desktop file and recreate it afterwards ? The
removal is useless: the buildroot has been cleaned at the beginning of %install
Line 60:       you don't need to install the desktop file in the buildroot
before calling desktop-file-install, it will take care of that.
Line 66:       you need to add category X-Fedora in the desktop file (wiki:
PackagingGuidelines#desktop)
Line 70:       don't call make install twice
Lines 71-73:   Just remove this dir since you add the doc with a %doc tag in the
%files list (it isn't there if you remove the additional make install anyway)
Lines 74-78:   this directory should be moved in %{_includedir}/cssed, where
include files are
Line 81:       for %clean, same thing as for %install. Just stick to rm -rf
%{buildroot}
Lines 87-119:  just use %{_datadir}/cssed. It's recursive.
Lines 121-126: use the %find_lang macro to list the translations (wiki:
PackageReviewGuidelines).
Lines 132-138: the include files should be in %_includedir/cssed
Line 139:      you don't need the docs in the -devel package. They are available
in the main package

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.

(In reply to comment #3)
> >> 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 :)

Alternative approach:

Use:
%dir %{_datadir}/cssed
%dir %{_datadir}/cssed/data
%dir %{_datadir}/cssed/pixmaps
%dir %{_datadir}/cssed/pixmaps

to own the directories non-recursively, then include the specific files as at
present.

I'd change %{_datadir}/man/man1/cssed.1.gz to %{_mandir}/man1/cssed.1*

The -devel package should own directory {_datadir}/cssed/include

(In reply to comment #3)
> 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 ?

The spec in the src.rpm is named cssed-0.4.0.spec at the moment. Just rename it
to cssed.spec before building the next version.

> Is this mandatory ?

Not at all, but then own directories as detailed by Paul in comment 4.

> Alternative approach:
> Use:
> %dir %{_datadir}/cssed
> %dir %{_datadir}/cssed/data
> %dir %{_datadir}/cssed/pixmaps
> %dir %{_datadir}/cssed/pixmaps
> to own the directories non-recursively, then include the specific files as at
> present.

Looks much better to me, thanks Paul. Fixed.

> I'd change %{_datadir}/man/man1/cssed.1.gz to %{_mandir}/man1/cssed.1*

Agree. Fixed.

> The -devel package should own directory {_datadir}/cssed/include

Fixed


All reviewed items in #4 fixed.


> The spec in the src.rpm is named cssed-0.4.0.spec at the moment.
> Just rename it to cssed.spec before building the next version.

Ah! I see, sorry. It's pending and will be fixed as soon as I know all the
changes needed in the upstream package.

Pending from #2:

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

Pending from #5:

- The spec in the src.rpm is named cssed-0.4.0.spec at the moment.
  PENDING to push changes to upstream package.


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

Old specs reacheable at:
http://iagorubio.com/fedora/cssed.spec.bak-1 (posted on #1)
http://iagorubio.com/fedora/cssed.spec.bak-2 (posted on #3)

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

Just tested, and builds in mock.

The requested changes are made but the package is not released yet, pending for
further comments. The Source tag in the following spec is not valid yet.

The changes have been commited to cssed cvs.


Fixed issues:

- Lines 74-78:   this directory should be moved in %{_includedir}/cssed
  Fixed in spec and upstream package.

- The spec in the src.rpm is named cssed-0.4.0.spec at the moment.
  Fixed.


The source rpm rebuilds fine.
The rpm package installs and uninstalls cleanly.
The -devel package installs and uninstalls cleanly.
The source rpm builds in mock.

rpmlint only complains about missing docs in -devel.
W: cssed-devel no-documentation

http://iagorubio.com/fedora/cssed.spec
http://iagorubio.com/fedora/cssed-0.4.1-0.src.rpm
http://iagorubio.com/fedora/cssed-0.4.1.tar.gz
 

IMO, the devel package is useless, because the package doesn't ship the
implementation corresponding to the API these headers specify.

AFAIS, these headers actually are private headers and should not be installed
(bug in Makefile.am) nor shipped.

The devel package, is only required to build cssed plugins, The implementation
in this case is the cssed executable itself.

The headers distributed contains the API "exported" by the application.

If to ship the devel package is a problem I can remove it from the spec file,
quite easy. There's only a need to remove --with-plugins-headers from the
%configure line and the related %package, %description and %files sections. 

The problem is it will be impossible to package the plugins as they could not be
rebuilt in a system without the pkg-config file, and the headers contained in
the devel package.

The plugins are not included in cssed and are shipped as separate packages.

Fair is to say that a couple of files distributed in this package have been
deprecated and are shipped just for compatibility purposes.

The amount of files needed to develop cssed plugins will be reduced to the
plugin.h file and the scintilla headers in further releases.

(In reply to comment #10)
> The devel package, is only required to build cssed plugins, The implementation
> in this case is the cssed executable itself.
>
> The headers distributed contains the API "exported" by the application.
Apparently wrong.

Proof: document.h is being installed and declares a lot of functions.
None of them is exported from any binary.

> If to ship the devel package is a problem I can remove it from the spec file,
> quite easy.

Presence of useless files is not an actual problem, it's the fact they are
meaningless and that your package's plugin API is a very doubtful (IMSNHO: bad)
design.

> Apparently wrong.

> Proof: document.h is being installed and declares a lot of functions.

Those are some of the deprecated functions shipped for backward compatibility I
mentioned in my comment #10.
http://cssed.sourceforge.net/plugins-doc/accesing_docs.html

> None of them is exported from any binary.

As you know, G_MODULE_EXPORT can be considered boilerplate on Unix and Linux
systems as it's a macro that expands to nothing.

Only on Windows it have a meaning as it expands to __declspec(dllexport).

So this functions can still be accessed by Linux, Unix and OsX plugins despite
the lack of the G_MODULE_EXPORT macro.

This file is here for those in-house developers that used them in their plugins,
to give them a time frame to change those deprecated functions.

No cssed plugin uses any of the deprecated interface functions.

> Presence of useless files is not an actual problem, it's the fact they are
> meaningless and that your package's plugin API is a very doubtful (IMSNHO:
> bad) design.

Well, I cannot argue about that with you, and it's just the second release of
cssed exporting an API so it's sure it's not mature. In fact in the
documentation it's stated it's a draft.
http://cssed.sourceforge.net/plugins-doc/introduction.html#whos_this_for

But please let me point that you're judging a set of deprecated functions as
part of the API, and you're not taking into account when you stated they're
useless, that it have its uses for the reasons I stated above.

The actual API is contained in plugin.h. cssedwindow.h only contains definitions
and the other files - taking out scintilla headers - are the same case as
document.h.

As I said, I can take out the devel package - so take out all plugins if they're
a concern - or clean up the include directory to delete the deprecated functions.

If any other solution is required please let me know.

Closing that due to lack of interest.
New request has been submitted in bug 221727.

*** This bug has been marked as a duplicate of 221727 ***