Bug 868578

The build doesn't respect Fedora's compiler flags: http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

Please remove the defattrs, as they are the default.

If you don't go for EPEL 5, remove the clean section, the rm in the install section and the buildroot definition.

You don't need the LICENSE file in the devel package. Don't know whether the README is useful there.

The description is very long and a bit like documentation in some sections.

Thanks Volker for the review. I will have a look at it shortly and publish the amended versions of the package. For the compiler flags (you are right; thanks to have spotted it), I will have some work to do, though, as the Makefile needs to be patched.

Apparently, upstream is no longer maintained (there has been no activity for 2 years already). However, RE2 seems to have reached a mature enough state, thus avoiding the need to be actively maintained...

Note that I will strive to maintain that package also on EPEL (5 and 6).

Spec URL: http://denisarnaud.fedorapeople.org/re2/re2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/re2/re2-0.0.0-2.fc17.src.rpm

[For later reference, I just add the corresponding URLs of the packaging guidelines]


(In reply to comment #1)
> The build doesn't respect Fedora's compiler flags:
> http://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags

You are right. I found a work around, redefining the CXXFLAGS and LDFLAGS environment variables. It may not be the cleanest way to do it, but I avoided to have to patch the Makefile in the source tar-ball. If you have a better idea, do not hesitate.


> Please remove the defattrs, as they are the default.

Reference: http://fedoraproject.org/wiki/Packaging:Guidelines#File_Permissions

You are right. Done


> If you don't go for EPEL 5, remove the clean section, the rm in the install
> section and the buildroot definition.

I intend to package re2 for EPEL (5 and 6) as well.


> You don't need the LICENSE file in the devel package. Don't know whether the
> README is useful there.

Reference:
http://fedoraproject.org/wiki/Packaging:LicensingGuidelines#Subpackage_Licensing
http://fedoraproject.org/wiki/Packaging:Guidelines#Duplicate_Files

You are right. As a consequence, there is no %doc files in the -devel sub-package, and rpmlint is not happy with that (it issues a warning). But I believe that we can live with that :)

 
> The description is very long and a bit like documentation in some sections.

You are right. I reduced it while keeping the essential I believe.

Volker, did you intend to do a full review?

Otherwise, I'm interested in taking it from here.

Denis, just a quick note meanwhile, why 0.0.0 as the version? Why not use the tarball version from:
https://code.google.com/p/re2/downloads/list

Please go ahead, I was just commenting.

(In reply to comment #4)
> Denis, just a quick note meanwhile, why 0.0.0 as the version? Why not use
> the tarball version from:
> https://code.google.com/p/re2/downloads/list

Thanks, Mathieu!

Indeed, I did not know that page (Google Code download site)... That is why I used SourceForge. I will now use, for sure not before the week-end. In the meantime, do not hesitate to go on with the (remaining part of the) review.

Spec URL: http://denisarnaud.fedorapeople.org/re2/re2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/re2/re2-20130115-1.fc18.src.rpm

As suggested in comment #4, that new version uses the (latest) tarball from the Google project repository (http://code.google.com/p/re2/downloads/list).

Ready for a new review :)

A couple of (non-blocker) comments first. 

* What is this empty comment line at the top of the spec file? Did you want to add something but forgot?

* Try to remove trailing spaces. There's one on the Group: line.

* The file re2/testing/unicode_test.py has a wrong shebang (#!/usr/bin/python2.4), however it doesn't seem to be used anywhere (not even during unit tests) and isn't installed, so that can probably be ignored.

* Why do you both export the CXXFLAGS/LDFLAGS and then pass them again on the make command line? Exporting doesn't seem to do anything, they need to be passed to the command, so you could just do:
-----
CXXFLAGS="${CXXFLAGS:-%optflags}"
LDFLAGS="${LDFLAGS:-%__global_ldflags}"
make %{?_smp_mflags} CXXFLAGS="$CXXFLAGS" LDFLAGS="$LDFLAGS" includedir=%{_includedir} libdir=%{_libdir}
-----

* The devel package requires the main package, as such it doesn't need to include the README and LICENSE files. Consider removing them.

* The %description for the main package is still **very** long. How about something like this instead:
-----
RE2 is a C++ library providing a fast, safe, thread-friendly alternative to
backtracking regular expression engines like those used in PCRE, Perl, and
Python.

Backtracking engines are typically full of features and convenient
syntactic sugar but can be forced into taking exponential amounts of
time on even small inputs.

In contrast, RE2 uses automata theory to guarantee that regular expression
searches run in time linear in the size of the input, at the expense of some
missing features (e.g back references and generalized assertions).
-----

* The %description of the devel subpackage talks about "development helper tools", but there aren't any. Consider removing the misleading statement.

Now for the actual review...


Summary of issues
=================

[!]: Requires correct, justified where necessary.
    => Drop the "Requires: pkgconfig" from the devel subpackage, it doesn't
       provide any pkgconfig file.

[!]: Rpmlint is run on all rpms the build produces.
    => There are a few "undefined-non-weak-symbol" warnings, which should be
       fixed. (try "ldd -d -r /usr/lib64/libre2.so.0.0.0" for more details)
       Adding "-pthread" to the CXXFLAGS seems to be enough. However, if this
       flags really is necessary, I would strongly suggest you submit a patch
       upstream so that it gets added to the RE2_CXXFLAGS in the Makefile (as
       these seem to be the minimum flags which shouldn't be overwritten)
    => The rest of the warnings can be safely ignored.

[!]: Packages should try to preserve timestamps of original installed files.
    => Try passing the INSTALL="install -p" variable to the make install command.

Details
=======

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: ldconfig called in %post and %postun if required.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.
[x]: Development (unversioned) .so files in -devel subpackage, if present.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
     Note: rm -rf %{buildroot} present but not required
    => Submitter indicated their intention to maintain EL 5 packages, for
       which this is necessary.

[x]: Sources contain only permissible code or content.
[-]: Each %files section contains %defattr if rpm < 4.4
[-]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[-]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Fully versioned dependency in subpackages, if present.
[!]: Package complies to the Packaging Guidelines
    => See outstanding issues.

[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
[x]: License file installed when any subpackage combination is installed.
[x]: Package consistently uses macro (instead of hard-coded directory
     names).
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[!]: Requires correct, justified where necessary.
    => Drop the "Requires: pkgconfig" from the devel subpackage, it doesn't
       provide any pkgconfig file.

[!]: Rpmlint is run on all rpms the build produces.
    => There are a few "undefined-non-weak-symbol" warnings, which should be
       fixed. (try "ldd -d -r /usr/lib64/libre2.so.0.0.0" for more details)
       Adding "-pthread" to the CXXFLAGS seems to be enough. However, if this
       flags really is necessary, I would strongly suggest you submit a patch
       upstream so that it gets added to the RE2_CXXFLAGS in the Makefile (as
       these seem to be the minimum flags which shouldn't be overwritten)
    => The rest of the warnings can be safely ignored.

[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[-]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[-]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 20480 bytes in 6 files.
[x]: Packages must not store files under /srv, /opt or /usr/local


===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
     Note: Buildroot: present but not needed
    => Submitter indicated their intention to maintain EL 5 packages, for
       which this is necessary.

[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
     Note: %clean present but not required
    => Submitter indicated their intention to maintain EL 5 packages, for
       which this is necessary.

[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: The placement of pkgconfig(.pc) files are correct.
[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[x]: %check is present and all tests pass.
[!]: Packages should try to preserve timestamps of original installed files.
    => Try passing the INSTALL="install -p" variable to the make install command.

[x]: Spec use %global instead of %define.


===== EXTRA items =====

Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Spec file according to URL is the same as in SRPM.
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.

Rpmlint
-------
Checking: re2-20130115-1.fc19.src.rpm
          re2-debuginfo-20130115-1.fc19.x86_64.rpm
          re2-20130115-1.fc19.x86_64.rpm
          re2-devel-20130115-1.fc19.x86_64.rpm
re2.src: W: spelling-error %description -l en_US automata -> automate, automaton, automatic
re2.src: W: spelling-error %description -l en_US multi -> mulch, mufti
re2.src: W: spelling-error %description -l en_US backreferences -> back references, back-references, references
re2.src: W: spelling-error %description -l en_US irregexp -> regexp, expire
re2.x86_64: W: spelling-error %description -l en_US automata -> automate, automaton, automatic
re2.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
re2.x86_64: W: spelling-error %description -l en_US backreferences -> back references, back-references, references
re2.x86_64: W: spelling-error %description -l en_US irregexp -> regexp, expire
4 packages and 0 specfiles checked; 0 errors, 8 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint re2-debuginfo re2 re2-devel
re2.x86_64: W: spelling-error %description -l en_US automata -> automate, automaton, automatic
re2.x86_64: W: spelling-error %description -l en_US multi -> mulch, mufti
re2.x86_64: W: spelling-error %description -l en_US backreferences -> back references, back-references, references
re2.x86_64: W: spelling-error %description -l en_US irregexp -> regexp, expire
re2.x86_64: W: undefined-non-weak-symbol /usr/lib64/libre2.so.0.0.0 pthread_rwlock_rdlock
re2.x86_64: W: undefined-non-weak-symbol /usr/lib64/libre2.so.0.0.0 pthread_rwlock_wrlock
re2.x86_64: W: undefined-non-weak-symbol /usr/lib64/libre2.so.0.0.0 pthread_rwlock_destroy
re2.x86_64: W: undefined-non-weak-symbol /usr/lib64/libre2.so.0.0.0 pthread_rwlock_init
re2.x86_64: W: undefined-non-weak-symbol /usr/lib64/libre2.so.0.0.0 pthread_rwlock_unlock
3 packages and 0 specfiles checked; 0 errors, 9 warnings.
# echo 'rpmlint-done:'

Requires
--------
re2-debuginfo-20130115-1.fc19.x86_64.rpm (rpmlib, GLIBC filtered):

re2-20130115-1.fc19.x86_64.rpm (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libm.so.6()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    rtld(GNU_HASH)

re2-devel-20130115-1.fc19.x86_64.rpm (rpmlib, GLIBC filtered):
    libre2.so.0()(64bit)
    pkgconfig
    re2(x86-64) = 20130115-1.fc19


Provides
--------
re2-debuginfo-20130115-1.fc19.x86_64.rpm:
    re2-debuginfo = 20130115-1.fc19
    re2-debuginfo(x86-64) = 20130115-1.fc19

re2-20130115-1.fc19.x86_64.rpm:
    libre2.so.0()(64bit)
    re2 = 20130115-1.fc19
    re2(x86-64) = 20130115-1.fc19

re2-devel-20130115-1.fc19.x86_64.rpm:
    re2-devel = 20130115-1.fc19
    re2-devel(x86-64) = 20130115-1.fc19

MD5-sum check
-------------
http://re2.googlecode.com/files/re2-20130115.tgz :
  CHECKSUM(SHA256) this package     : a95d83ed8203817587f7cc368b6f7199d8a4b1558372c306b07201058b92e1fe
  CHECKSUM(SHA256) upstream package : a95d83ed8203817587f7cc368b6f7199d8a4b1558372c306b07201058b92e1fe

Thanks Mathieu for that thorough review. I am working on integrating your feedback.

In the meantime, I just figured out that an old version of that package is already in the Fedora repository: http://admin.fedoraproject.org/pkgdb/acls/name/re2c

So, I will try to work with the maintainer, namely thias, to upgrade that package to the newest version if he so likes. Otherwise, I will go on submitting that newest version here.

(In reply to comment #9)
> In the meantime, I just figured out that an old version of that package is
> already in the Fedora repository:
> http://admin.fedoraproject.org/pkgdb/acls/name/re2c

Please disregard that comment, as re2c and re2 are different projects with different maintainers.

Spec URL: http://denisarnaud.fedorapeople.org/re2/re2.spec
SRPM URL: http://denisarnaud.fedorapeople.org/re2/re2-20130115-2.fc18.src.rpm

==============================================
(In reply to comment #8)
> Now for the actual review...
> 
> [!]: Requires correct, justified where necessary.
>     => Drop the "Requires: pkgconfig" from the devel subpackage, it doesn't
>        provide any pkgconfig file.

Done


> [!]: Rpmlint is run on all rpms the build produces.
>     => There are a few "undefined-non-weak-symbol" warnings, which should be
>        fixed. (try "ldd -d -r /usr/lib64/libre2.so.0.0.0" for more details)
>        Adding "-pthread" to the CXXFLAGS seems to be enough. However, if this
>        flags really is necessary, I would strongly suggest you submit a patch
>        upstream so that it gets added to the RE2_CXXFLAGS in the Makefile (as
>        these seem to be the minimum flags which shouldn't be overwritten)
>     => The rest of the warnings can be safely ignored.

Done. It has been suggested upstream to have an additional RE2_LDFLAGS variable in the Makefile (http://code.google.com/p/re2/source/browse/Makefile): http://groups.google.com/forum/?fromgroups=#!topic/re2-dev/bkUDtO5l6Lo


> [!]: Packages should try to preserve timestamps of original installed files.
>     => Try passing the INSTALL="install -p" variable to the make install
> command.

Done

=================

Ready for the final review/approval :)

Differences between this new submission and the previous one:
----------
--- 868578-re2.old/srpm/re2.spec	2013-02-14 12:50:50.829422617 +0800
+++ 868578-re2/srpm/re2.spec	2013-02-18 11:44:51.036537713 +0800
@@ -1,64 +1,52 @@
-#
 Name:           re2
 Version:        20130115
-Release:        1%{?dist}
+Release:        2%{?dist}
 
 Summary:        C++ fast alternative to backtracking RE engines
 
-Group:          System Environment/Libraries 
+Group:          System Environment/Libraries
 License:        BSD
 URL:            http://code.google.com/p/%{name}/
 Source0:        http://re2.googlecode.com/files/%{name}-%{version}.tgz
 BuildRoot:      %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
 
 %description
-RE2 is a fast, safe, thread-friendly alternative to backtracking
-regular expression engines like those used in PCRE, Perl, and
-Python. It is a C++ library.
-
-Backtracking engines are typically full of features and convenient
-syntactic sugar but can be forced into taking exponential amounts of
-time on even small inputs. RE2 uses automata theory to guarantee that
-regular expression searches run in time linear in the size of the
-input. RE2 implements memory limits, so that searches can be
-constrained to a fixed amount of memory. RE2 is engineered to use a
-small fixed C++ stack footprint no matter what inputs or regular
-expressions it must process; thus RE2 is useful in multi-threaded
-environments where thread stacks cannot grow arbitrarily large.
-
-On large inputs, RE2 is often much faster than backtracking engines;
-its use of automata theory lets it apply additional optimization that
-the others cannot.
-
-RE2 supports sub-match extraction, but not back references.
-
-If you absolutely need backreferences and generalized assertions, then
-RE2 is not for you, but you might be interested in irregexp, Google
-Chrome's regular expression engine.
+RE2 is a C++ library providing a fast, safe, thread-friendly alternative to
+backtracking regular expression engines like those used in PCRE, Perl, and
+Python.
+
+Backtracking engines are typically full of features and convenient syntactic
+sugar but can be forced into taking exponential amounts of time on even small
+inputs.
+
+In contrast, RE2 uses automata theory to guarantee that regular expression
+searches run in time linear in the size of the input, at the expense of some
+missing features (e.g back references and generalized assertions).
 
 %package        devel
-Summary:        Header files, libraries and development helper tools for %{name}
+Summary:        C++ header files and library symbolic links for %{name}
 Group:          Development/Libraries
 Requires:       %{name}%{?_isa} = %{version}-%{release}
-Requires:       pkgconfig
 
 %description    devel
-This package contains the header files, shared libraries and
-development helper tools for %{name}. If you would like to develop
-programs using %{name}, you will need to install %{name}-devel.
+This package contains the C++ header files and symbolic links to the shared
+libraries for %{name}. If you would like to develop programs using %{name},
+you will need to install %{name}-devel.
 
 
 %prep
 %setup -q -n %{name}
 
 %build
-CXXFLAGS="${CXXFLAGS:-%optflags}"; export CXXFLAGS
-LDFLAGS="${LDFLAGS:-%__global_ldflags}"; export LDFLAGS
-make %{?_smp_mflags} CXXFLAGS='%optflags' LDFLAGS='%__global_ldflags' includedir=%{_includedir} libdir=%{_libdir}
+# The -pthread flag issue has been submitted upstream:
+# http://groups.google.com/forum/?fromgroups=#!topic/re2-dev/bkUDtO5l6Lo
+CXXFLAGS="${CXXFLAGS:-%optflags}"
+LDFLAGS="${LDFLAGS:-%__global_ldflags} -pthread"
+make %{?_smp_mflags} CXXFLAGS="$CXXFLAGS" LDFLAGS="$LDFLAGS" includedir=%{_includedir} libdir=%{_libdir}
 
 %install
 rm -rf $RPM_BUILD_ROOT
-make install DESTDIR=$RPM_BUILD_ROOT includedir=%{_includedir} libdir=%{_libdir}
+make install INSTALL="install -p" DESTDIR=$RPM_BUILD_ROOT includedir=%{_includedir} libdir=%{_libdir}
 
 # Suppress the static library
 find $RPM_BUILD_ROOT -name 'lib%{name}.a' -exec rm -f {} \;
@@ -78,12 +66,14 @@
 %{_libdir}/lib%{name}.so.*
 
 %files devel
-%doc LICENSE README
 %{_includedir}/%{name}
 %{_libdir}/lib%{name}.so
 
 
 %changelog
+* Sun Feb 17 2013 Denis Arnaud <denis.arnaud_fedora> 20130115-2
+- Took into account the feedback from review request (#868578).
+
 * Sun Feb 10 2013 Denis Arnaud <denis.arnaud_fedora> 20130115-1
 - The download source comes now directly from the project.
 
----------

This addresses all the issues I had with the previous package.

Approved.

Many thanks Mathieu!
-----------------------------

New Package SCM Request
=======================
Package Name: re2
Short Description: C++ fast alternative to backtracking RE engines
Owners: denisarnaud
Branches: f17 f18 f19 el5 el6
FAS username: denisarnaud
-----------------------------

Git done (by process-git-requests).

re2-20130115-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/re2-20130115-2.fc18

re2-20130115-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/re2-20130115-2.fc17

re2-20130115-2.el6 has been submitted as an update for Fedora EPEL 6.
https://admin.fedoraproject.org/updates/re2-20130115-2.el6

re2-20130115-2.el5 has been submitted as an update for Fedora EPEL 5.
https://admin.fedoraproject.org/updates/re2-20130115-2.el5

re2-20130115-2.el6 has been pushed to the Fedora EPEL 6 testing repository.

re2-20130115-2.fc17 has been pushed to the Fedora 17 stable repository.

re2-20130115-2.fc18 has been pushed to the Fedora 18 stable repository.

re2-20130115-2.el6 has been pushed to the Fedora EPEL 6 stable repository.

re2-20130115-2.el5 has been pushed to the Fedora EPEL 5 stable repository.