Spec URL: http://denisarnaud.fedorapeople.org/re2/re2.spec SRPM URL: http://denisarnaud.fedorapeople.org/re2/re2-0.0.0-1.fc17.src.rpm Fedora Account System Username: denisarnaud 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 multithreaded 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 optimizations that the others cannot. Unlike most automata-based engines, RE2 implements almost all the common Perl and PCRE features and syntactic sugars. It also finds the leftmost-first match, the same match that Perl would, and can return submatch information. The one significant exception is that RE2 drops support for backreferences¹ and generalized zero-width assertions, because they cannot be implemented efficiently. The syntax page gives full details. For those who want a simpler syntax, RE2 has a POSIX mode that accepts only the POSIX egrep operators and implements leftmost-longest overall matching. To get started writing programs that use RE2, see the C++ API description. For details about the implementation, see "Regular Expression Matching in the Wild." Technical note: there's a difference between submatches and backreferences. Submatches let you find out what certain subexpressions matched after the match is over, so that you can find out, after matching dogcat against (cat|dog)(cat|dog), that \1 is dog and \2 is cat. Backreferences let you use those subexpressions during the match, so that (cat|dog)\1 matches catcat and dogdog but not catdog or dogcat. RE2 supports submatch extraction, but not backreferences. 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.
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.