Please do not reply directly to this email. All additional comments should be made in the comments box of this bug. https://bugzilla.redhat.com/show_bug.cgi?id=486804 --- Comment #3 from Ben Martin <monkeyiq@xxxxxxxxxxxx> 2009-03-17 05:48:25 EDT --- Firstly, thank you for your indepth and very informative comments! A few issues like not having split out a -devel package I forgot to do before submitting and apologize for that. I should mention up front that I've been using this spec on the openSUSE build system (OBS) to make rpms for Fedora and openSUSE. If possible I'd like to continue using the same spec in Fedora and in the OBS to make openSUSE rpms too. I'm not sure if this is possible or not, but I can always hope. I've updated the spec file and src.rpm file with changes from your comments. This should also make the next spec less of a hassle because I'll have already addressed these comments. > Might be that I've caught all issues, but a lot of work is needed to bring this > into shape: > > > * Run "rpmlint -i" on your src.rpm and also on all built packages. Try to fix > as many Warnings and Errors as plausible. > There are still a few issues rpmlint picks up. The main one is how to use multiple licenses properly. > > > License: GPL > > This is not just an invalid value for the "License" tag, it is inaccurate. Some > source files mention "GPLv2+", some the "Boost Software License 1.0". Others > contain a "Copyright Only" header: > https://fedoraproject.org/wiki/Licensing/CopyrightOnly > The "macros/ferrismacros.m4" file contains pieces licenced under the "LGPLv2+". > > https://fedoraproject.org/wiki/Licensing OK, I've taken a look at these to see if I could consolidate things (if I owned copyright on the file & code) but it looks like we'll need these three liceneses unless I plan to rewrite some already working code. So now we have: License: LGPLv2+, Boost, Copyright only I'm not sure how to chain them though. rpmlint doesn't seem to like comma separated and rpmbuild doesn't like multiple tags. > > > > > Source: http://prdownloads.sourceforge.net/witme/%{name}-%{version}.tar.bz2 > > There are special guidelines for Sourceforge.net download locations: > https://fedoraproject.org/wiki/Packaging/SourceURL#Sourceforge.net Excellent. The above URL was one I came up with ages ago, though it is surprisingly close to the Fedora recommendation. I've updated to fix this inline with the URL above. > > > > Packager: Ben Martin <monkeyiq@xxxxxxxxxxxxxxxxxxxxx> > > Don't set this tag. The build-system will do it. In general, be careful with > hardcoding "Packager"/"Vendor" tags in spec files you release. There are people > who build broken binary rpms, which would appear as if they have been built by > you, because they contain your name in the "Packager" tag. The spec %changelog > is less of a problem in case you wonder. Removed. Though I also plan to try to use the same .spec with OBS to make openSUSE rpms, so I might have to add it back conditionally. > > > > BuildRequires: gcc-c++ > > Redundant, as the C++ compiler is available in the minimal build environment: > https://fedoraproject.org/wiki/Packaging:Guidelines#Exceptions_2 Ah, if this redundancy is OK can we leave it? To build with OBS you have to include the compiler too so I need that redundant line for the spec to with on OBS too. > > > > if [ "$SMP" != "" ]; then > > (make "MAKE=make -k -j $SMP"; exit 0) > > make > > else > > make > > fi > > At least with Fedora, you can replace this with just: > > make %{?_smp_mflags} > Done. If openSUSE doesn't actually set _smp_mflags I might add some glue in the header of the spec to set it properly on OBS. > > > %install > > %makeinstall > > First command in %install section must be: rm -rf $RPM_BUILD_ROOT > (or "rm -rf %buildroot" if you prefer the lower case macro everywhere) > > make DESTDIR="$RPM_BUILD_ROOT" install > or: > make DESTDIR="$RPM_BUILD_ROOT" INSTALL="install -p" > > shall be preferred over %makeinstall. This is one area where I thought using the macro was the golden thing to do. Changed to the second version with the modification you included. > > > > %files > > %defattr(-,root,root,0755) > > Doesn't %defattr(-,root,root,-) work? > > > %doc AUTHORS README COPYING ChangeLog INSTALL > > Typically, the standard file "INSTALL" is irrelevant to RPM package users. Here > it is empty even. Fixed. > > > %{_libdir}/* > > %{_includedir}/* > > Package must be split into a main library pkg and a ferrisloki-devel > sub-package, which contains the files needed only for software development > (i.e. the *.so symlink and the headers). > > %{_libdir}/* includes too many files it must not include (e.g. the debuginfo > files). Use at most %{_libdir}/*.so.* for the main pkg and %{_libdir}/*.so > for the -devel subpkg. > > > > -rw-r--r-- /usr/lib/libferrisloki.a > > -rwxr-xr-x /usr/lib/libferrisloki.la > > Don't build/include these. > https://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries > > > > -rw-r--r-- /usr/include/FerrisLoki/Extensions.cpp > > -rw-r--r-- /usr/include/FerrisLoki/OrderedStatic.cpp > > -rw-r--r-- /usr/include/FerrisLoki/SafeFormat.cpp > > -rw-r--r-- /usr/include/FerrisLoki/Singleton.cpp > > -rw-r--r-- /usr/include/FerrisLoki/SmallObj.cpp > > -rw-r--r-- /usr/include/FerrisLoki/SmartPtr.cpp > > -rw-r--r-- /usr/include/FerrisLoki/StrongPtr.cpp > > Hmm? OK I split the package to include a -devel package and distribute the libdir stuff as mentioned. The strange .cpp files are no longer packaged. > > > * The pkgconfig file ferrisloki.pc is questionable, because it is tuned for > static linking and does a few bad things: > > > Libs: -L${libdir} -lferrisloki -lsigc-2.0 > The shared library is linked with libsigc-2.0 already. No need to link again. > > > Requires: > > The pkgconfig dependency on "sigc++-2.0" is missing in this field if you really > want it to be a strict dependency - I doubt you want it. It would also add the > proper libsigc++20 CFLAGS and LDFLAGS automatically when running pkg-config, > and you would not need to add them to your .pc file manually. > > For platforms older than Fedora 11, the ferrisloki-devel subpackage must > "Requires: libsigc++20-devel", however see below. > > > Cflags: -I${includedir} -I${includedir}/FerrisLoki > > -I${includedir}/FerrisLoki/loki -I/usr/include/sigc++-2.0 > > -I/usr/lib/sigc++-2.0/include > > Why do add two search paths for FerrisLoki and FerrisLoki/loki? There are > several files which include <loki/...>, so the extra search path is not needed. > > The sigc++-2.0 related Cflags are redundant, if you would fill in the Requires > field correctly. However, only the boost extension uses sigc++20 headers. And > that extension would need "Requires: boost-devel" in the spec file. > > In other words, I don't see why the sigc++ stuff is in the .pc file at all. > Updated to remove the sigc++ stuff from the cflags and libs in the .pc and use Requires for both sigc++ and boost instead. The problem that lead to this is that I have added some extensions to the libferrisloki library beyond what the mainline Loki library has. These extensions are in %{_includedir}/FerrisLoki/*.hh and over time have grown to include sigc++ and boost support to make all the higher level libraries that use libferrisloki easier to write. Of course, if you don't #include any of these extensions then sigc++ and boost are not strictly needed by libferrisloki. On the other hand, since I'm the main (sole?) user of libferrisloki just adding both sigc++ and boost to the requires should make things cleaner. -- Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug. _______________________________________________ Fedora-package-review mailing list Fedora-package-review@xxxxxxxxxx http://www.redhat.com/mailman/listinfo/fedora-package-review