Hide Forgot
Spec URL: http://besser82.fedorapeople.org/review/stout.spec SRPM URL: http://besser82.fedorapeople.org/review/stout-0.1.1-2.2d3d1ab.fc19.src.rpm Description: Headers used for for development of sturdy applications, and leveraged by Mesos. Stout is a header only library that is contains a series of primitives to assist in the development of building sturdy C++ applications. Currently this application is leveraged by Mesos. Note: as that project has only headers (i.e., no library/binary object), this package (i.e., the -devel package) is the one containing all of the project. There's no package with a library to link for this. Fedora Account System Username: besser82
looks good, but hey I've already looked at it.
I'll take a look at this.
So I'm looking through this, and it appears to just be headers, and it doesn't build a library. From README.md: "You'll notice that the library is designed in a way that can lead to a lot of copying. This decision was deliberate. " How is this to be used? If other packages depend on it, how will security updates be handled? While this package doesn't seem to directly fall afoul of: https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries , I'd think that any package making use of it would run that risk. Comments?
Hi Jon! Thanks for review! (In reply to Jon Ciesla from comment #3) > So I'm looking through this, and it appears to just be headers, and it > doesn't build a library. > > From README.md: > > "You'll notice that the library is designed in a way that can lead to a > lot of copying. This decision was deliberate. " > > How is this to be used? A package using this will BR it. The headers are included like any other C++ header-files. > If other packages depend on it, how will security updates be handled? There's no rebuild neccessary. AFAIK this is same way some boost-headers (non-copyable.hpp, e.g.) are used. These headers are mostly including headers from other libs and the build binary will link against those. So there should be no trouble. > While this package doesn't seem to directly fall afoul > of: > > https://fedoraproject.org/wiki/Packaging:No_Bundled_Libraries > > , I'd think that any package making use of it would run that risk. > > Comments? Like I've written above.
It's no different from many other header only libraries that exist in the C++ landscape. Many exist in fedora. Explicit -devel dependencies would require tracking by the maintainers.
Boost often uses linked solibs. Is there another header-only library I can compare this to?
tclap is a very simple one, but I'm pretty sure if you searched around you can find more. But for ref, a vast majority of boost is header only.
I see. Ok. Good: - rpmlint checks return: stout.spec:17: W: macro-in-comment %{url} There is a unescaped macro after a shell style comment in the specfile. Macros are expanded everywhere, so check if it can cause a problem in this case and escape the macro with another leading % if appropriate. stout.spec:17: W: macro-in-comment %{commit} There is a unescaped macro after a shell style comment in the specfile. Macros are expanded everywhere, so check if it can cause a problem in this case and escape the macro with another leading % if appropriate. stout.spec:17: W: macro-in-comment %{name} There is a unescaped macro after a shell style comment in the specfile. Macros are expanded everywhere, so check if it can cause a problem in this case and escape the macro with another leading % if appropriate. stout.spec:17: W: macro-in-comment %{version} There is a unescaped macro after a shell style comment in the specfile. Macros are expanded everywhere, so check if it can cause a problem in this case and escape the macro with another leading % if appropriate. stout.spec:17: W: macro-in-comment %{shortcommit} There is a unescaped macro after a shell style comment in the specfile. Macros are expanded everywhere, so check if it can cause a problem in this case and escape the macro with another leading % if appropriate. stout.src: W: spelling-error %description -l en_US devel -> delve, devil, revel The value of this tag appears to be misspelled. Please double-check. devel-file-in-non-devel-package: Lots. These should all be in -devel, and the main package should be docs-only. - package meets naming guidelines - package meets packaging guidelines - license ( ASL 2.0 ) OK, text in %doc, matches source - spec file legible, in am. english - source matches upstream - package compiles on devel (x86_64) - no missing BR - no unnecessary BR - no locales - not relocatable - owns all directories that it creates - no duplicate files - permissions ok - %clean ok - macro use consistent - code, not content - no need for -docs - nothing in %doc affects runtime - no need for .desktop file - devel package ok - no .la files - post/postun ldconfig ok - devel requires base package n-v-r So fix up the commented macros and move the includes to -devel and you should be set.
It _is_ a -devel package already, albeit a virtual one: Provides: %{name}-devel = %{version}-%{release}
Updated spec/srpm %changelog * Tue Jul 30 2013 Björn Esser <bjoern.esser> - 0.1.1-3.2d3d1ab - move everything into devel-pkg and make it provide main-pkg - fix macros in comment * Thu Jul 25 2013 Björn Esser <bjoern.esser> - 0.1.1-2.2d3d1ab - use datadir instead of libdir for pkg-config - make package noarch again - removed %%{?_isa}-bits * Thu Jul 25 2013 Björn Esser <bjoern.esser> - 0.1.1-1.10f7b88 - new version - ships a pkg-config-file now, must be arched now - make install-target is supported - adding test-dir as examples - using autoreconf instead of bootstrap-script - disable building debuginfo - general clean-up and nuked trailing whitespaces - added needs for el5 - changed %%define --> %%global - dropped -devel-subpkg without main-pkg and make pkg provide -devel * Mon Jul 22 2013 Igor Gnatenko <i.gnatenko.brain> - 0.1.0-1.270dba8 - In release added git shotcommit - In BuildRequires added automake - Droped BuildRoot target (since Fedora 18 was deprecated) - Dropped %%clean section (since Fedora 18 was deprecated) - Dropped %%defattr directives (since Fedora 18 was deprecated) - Dropped %%files section (not needed) - other fixes * Mon Jul 22 2013 Timothy St. Clair <tstclair> 0.1.0-1 - initial fedora package ##### Spec URL: http://besser82.fedorapeople.org/review/stout.spec SRPM URL: http://besser82.fedorapeople.org/review/stout-0.1.1-3.2d3d1ab.fc19.src.rpm
Some remarks: 1. The package is incomplete or lacks a dep: stout/gtest.hpp accesses a file, which does not exist: include/stout/gtest.hpp:#include <gtest/gtest.h> 2. C++-header-only packages should exercise test suites, when a package provides such. This package provides a testsuite (examples), unfortunately these are nonbuildable. 3. The cpp-define usage of this package raises doubts on this package's code quality: E.g. a) Some header guards are non-"package"-prefixed: __NOTHING_HPP__ include/stout/nothing.hpp __PROCESS_PREPROCESSOR_HPP__ include/stout/preprocessor.hpp __PROC_HPP__ include/stout/proc.hpp All other header-guards are package-prefixed (They use: "__STOUT_"*) b) Some of the defines being used are likely to clash with other defines, e.g. CAT, REPEAT etc. in include/stout/preprocessor.hpp c) The package relies on autoconf defines in external headers: include/stout/gzip.hpp:#ifdef HAVE_LIBZ include/stout/net.hpp:#ifdef HAVE_LIBCURL This violates the autoconf working principles and needs to be fixed (REVIEW BLOCKER)
Good catches, Ralf. 1. certainly needs fixing, 2. if possible, and 3 for sure.
(In reply to Ralf Corsepius from comment #11) > Some remarks: > > 1. The package is incomplete or lacks a dep: > > stout/gtest.hpp accesses a file, which does not exist: > include/stout/gtest.hpp:#include <gtest/gtest.h> in the spec and the auto-tools there should be a call out for gtest-devel which certain does contain the header file <gtest/gtest.h> > > 2. C++-header-only packages should exercise test suites, when a package > provides such. This package provides a testsuite (examples), unfortunately > these are nonbuildable. > We can open a ticket upstream around *this. > > 3. The cpp-define usage of this package raises doubts on this package's code > quality: > > E.g. > a) Some header guards are non-"package"-prefixed: > __NOTHING_HPP__ include/stout/nothing.hpp > __PROCESS_PREPROCESSOR_HPP__ include/stout/preprocessor.hpp > __PROC_HPP__ include/stout/proc.hpp > > All other header-guards are package-prefixed (They use: "__STOUT_"*) > > b) Some of the defines being used are likely to clash with other defines, > e.g. > CAT, REPEAT etc. in include/stout/preprocessor.hpp > > c) The package relies on autoconf defines in external headers: > include/stout/gzip.hpp:#ifdef HAVE_LIBZ > include/stout/net.hpp:#ifdef HAVE_LIBCURL > > This violates the autoconf working principles and needs to be fixed (REVIEW > BLOCKER) We should definitely fix #3, I will try to get an update shortly.
re #1 - This is done in the spec, arguably we could probably add an auto check too, but we are still pending on our other pull requests. re #2 - I've opened an issue upstream (https://github.com/3rdparty/stout/issues/5) re #3 a.) Fixed b.) This is the price one pays in C++ there are many tricks folks can use to work around, but alas it is their library with its own dep-graph c.) Fixed srpm: http://tstclair.fedorapeople.org/mesos/stout/stout-0.1.1-2.a401792.fc18.src.rpm spec: http://tstclair.fedorapeople.org/mesos/stout/stout.spec
srm update - http://tstclair.fedorapeople.org/mesos/stout/stout-0.1.1-2.7c9d71c.fc18.src.rpm
Sorry for the delay, been busy. Looks good, other than the unescaped commented macros on line 17, which you can fix before import. APPROVED.
Like for any header-only package, I'd strongly recommend to change this package into an arch'ed src.package with BuildArch: noarch for the resulting binary packages. This would trigger building this package for all Fedora archs and would help catching arch-dependent bugs (IIRC, Jon, we once discussed this on an FPC meeting or on fedora-packaging@. Unfortunately, I don't recall the outcome).
+1 to Ralf's suggestion. Ralf, I believe you're right. Making more things arched can eat up mirror space, but if it makes things work more correctly, it's certainly the right way to go.
+1 FWIW, I've suggested that recently in the metslib review (another header-only API with a test-suite).
Thanks for the review, Jon! ##### (In reply to Ralf Corsepius from comment #17) > Like for any header-only package, I'd strongly recommend to change this > package into an arch'ed src.package with BuildArch: noarch for the resulting > binary packages. > > This would trigger building this package for all Fedora archs and would help > catching arch-dependent bugs (IIRC, Jon, we once discussed this on an FPC > meeting or on fedora-packaging@. Unfortunately, I don't recall the outcome). I'll change the spec on SCM import to do so. ##### New Package SCM Request ======================= Package Name: stout Short Description: C++ headers for building sturdy software Owners: besser82 tstclair ignatenkobrain Branches: el5 el6 f18 f19 InitialCC: bigdata
Git done (by process-git-requests). Did not add bigdata as InitialCC, not a valid FAS account.
stout-0.1.1-4.7c9d71c.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/stout-0.1.1-4.7c9d71c.el6
stout-0.1.1-4.7c9d71c.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/stout-0.1.1-4.7c9d71c.fc18
stout-0.1.1-4.7c9d71c.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/stout-0.1.1-4.7c9d71c.fc19
stout-0.1.1-5.7c9d71c.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/stout-0.1.1-5.7c9d71c.el6
stout-0.1.1-5.7c9d71c.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/stout-0.1.1-5.7c9d71c.fc18
stout-0.1.1-5.7c9d71c.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/stout-0.1.1-5.7c9d71c.fc19
(In reply to Ralf Corsepius from comment #17) > Like for any header-only package, I'd strongly recommend to change this > package into an arch'ed src.package with BuildArch: noarch for the resulting > binary packages. > > This would trigger building this package for all Fedora archs and would help > catching arch-dependent bugs (IIRC, Jon, we once discussed this on an FPC > meeting or on fedora-packaging@. Unfortunately, I don't recall the outcome). Needed changes were done in update to stout-0.1.1-5.7c9d71c. See above.
stout-0.1.2-1.099483f.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/stout-0.1.2-1.099483f.el5
stout-0.1.2-1.099483f.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/stout-0.1.2-1.099483f.el6
stout-0.1.2-1.099483f.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/stout-0.1.2-1.099483f.fc18
stout-0.1.2-1.099483f.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/stout-0.1.2-1.099483f.fc19
stout-0.1.2-1.099483f.fc19 has been pushed to the Fedora 19 testing repository.
liblinear-1.93-2.el5 has been submitted as an update for Fedora EPEL 5. https://admin.fedoraproject.org/updates/liblinear-1.93-2.el5
liblinear-1.93-2.el6 has been submitted as an update for Fedora EPEL 6. https://admin.fedoraproject.org/updates/liblinear-1.93-2.el6
liblinear-1.93-2.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/liblinear-1.93-2.fc18
liblinear-1.93-2.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/liblinear-1.93-2.fc19
stout-0.1.2-1.099483f.fc18 has been pushed to the Fedora 18 stable repository.
stout-0.1.2-1.099483f.fc19 has been pushed to the Fedora 19 stable repository.