Spec Name or Url: http://iagorubio.com/fedora/cssed.spec SRPM Name or Url: http://iagorubio.com/fedora/cssed-0.4.0-0.src.rpm Description: cssed is a tiny GTK+ CSS editor and validator for web developers.
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 ***