Bug 988545

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.