Spec URL: http://crow-designer.sourceforge.net/temp/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/poco-1.3.5-1.fc11.src.rpm Description: The POCO C++ Libraries (POCO stands for POrtable COmponents) are open source C++ class libraries that simplify and accelerate the development of network-centric, portable applications in C++. The POCO C++ Libraries are built strictly on standard ANSI/ISO C++, including the standard library. (This is the first submission.)
Hi Maxim. Glad to see another russian Fedora member. Regarding your submission - since it is in a very good shape, at least at a first glance, I'll review it ASAP (today or tomorrow). BTW feel free to join fedora-devel.ru if you have some technical questions.
Notes: * Current naming scheme looks unclear at least for me. Why not to split packages into %{name}-MySQL, %{name}-ODBC and %{name}-ZIP instead of %{name}-extra. * I don't fully understand why we need specific %{name}-debug package. We already have %{name}-debuginfo. In any case I think %{name}-debug should be splitted into %{name}-debug and %{name}-extra-debug packages (in conformance with main two packages). However, I don't insist you to fully satisfy these two requests, but you should comment them. * "BuildRequires: /usr/bin/perl" - why not to use "BuildRequires: perl" ? Other things looks sane. Here is a koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1429077 * rpmlint is almost silent (two warnings may be ignored): [petro@Workplace Desktop]$ rpmlint poco-* poco-debug.i586: W: no-documentation poco-extra.i586: W: no-documentation 4 packages and 0 specfiles checked; 0 errors, 2 warnings. [petro@Workplace Desktop]$
Hello, and thank you for a quick reply! *1. The proposed split has a simple rationale - move rarely used features to separate packages and keep the total number of packages to the minimum: 'poco' - contains retail builds of the general-purpose libraries 'poco-extra' - retail builds of the libraries that are either rarely used or pull peculiar dependencies 'poco-debug' - debug builds (POCO makefiles produce them) of all libraries, they are used only during development of the POCO-based applications for testing purposes 'poco-devel' - headers and .so links to retail and debug libraries 'poco-debuginfo' - automatically generated package with debug-related information But I agree to further split 'poco-extra' into three separate 'poco-mysql', 'poco-odbc' and 'poco-zip'. This variant (although requiring increased typing from the .spec compositor) is more reasonable. *2. I am not sure that 'poco-debug' has to be split because 'poco-devel', as a development package, should pull everything anyway (see http://packages.debian.org/squeeze/libpoco-dev which partially demonstrates this). But it may be renamed to 'poco-testing' in order to avoid associations with 'poco-debuginfo'. *3. The perl build dependency is mentioned as "/usr/bin/perl" instead of "perl" in order to emphasize the dependency on a particular file (a CLI program in this case) but not on a package; probably the same situation as with "/sbin/ldconfig" being used instead of "glibc".
Spec URL: http://crow-designer.sourceforge.net/temp/2/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/2/poco-1.3.5-2.fc11.src.rpm CHANGES ======= - The 'poco-extra' subpackage was split into separate 'poco-odbc', - 'poco-mysql' and 'poco-zip'. The 'poco-debug' subpackage was renamed - to 'poco-testing'. The 'poco-doc' subpackage with the API reference - documentation was added. RPMLINT ======= $ rpmlint * poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/styles.css poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/prettify.css poco-mysql.x86_64: W: no-documentation poco-odbc.x86_64: W: no-documentation poco-testing.x86_64: W: no-documentation poco-zip.x86_64: W: no-documentation 8 packages and 0 specfiles checked; 0 errors, 6 warnings. $
When I try to install the src rpm via rpm -i ~/rombuld/SRPMS/poco-1.3.5-2.fc11.src.rpm I get this: rpm -i rpmbuild/SRPMS/poco-1.3.5-2.fc11.src.rpm --- Warnung: Benutzer maxim existiert nicht - benutze Root Warnung: Gruppe maxim existiert nicht - benutze Root Warnung: Benutzer maxim existiert nicht - benutze Root Warnung: Gruppe maxim existiert nicht - benutze Root Warnung: Benutzer maxim existiert nicht - benutze Root Warnung: Gruppe maxim existiert nicht - benutze Root --- which tells me that some files have your personal user access rights, and as they do not exist here they get replaced with root. This should be fixed sometime.
(In reply to comment #5) > This should be fixed sometime. No, it shouldn't. That's a normal behavior, since all new rpms are builded in ~/ and, therefore, have uid and gid of user, who built them (except someone builds them as root).
Hello, I'd like to see poco in Fedora. A project I'd like to package uses it. Has this review stalled? Are we just waiting for a sponsor here?
(In reply to comment #7) > Hello, > I'd like to see poco in Fedora. A project I'd like to package uses it. > Has this review stalled? Are we just waiting for a sponsor here? Hello, Michal! Unfortunately, I have no spare time to complete this review. Feel free to re-assign this to yourself. Regardins sponsorship - I'll sponsor Maxim, when your review ill be finished.
s/"ill be"/"will be"/
OK, I'm reassigning the review to myself.
Maxim, For now I have just a couple of suggestions: - Please do not use the "-s" option for make. Full logs from the compilation should always be present in Koji builds. - You don't have to use perl to do simple substitutions. sed is capable enough and it's always present in the buildroots. For example: sed -i.orig -e 's|$(INSTALLDIR)/lib\b|$(INSTALLDIR)/%{_lib}|g' Makefile The other two substitutions use the same script, so they can even be merged into a single sed invocation. Oh, and %{_libdir} is nicer than /usr/%{_lib}.
Spec URL: http://crow-designer.sourceforge.net/temp/3/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/3/poco-1.3.5-3.fc11.src.rpm CHANGES ======= - Each POCO component is now put in its own binary package. The 'poco' package is now a meta package. - Option "-s" was removed from the "make" invocation commands. - "perl" was replaced by "sed" for string substitutions in Makefile's. RPMLINT ======= $ rpmlint *.rpm poco.x86_64: E: devel-dependency poco-devel poco.x86_64: E: no-binary poco-crypto.x86_64: W: no-documentation poco-data.x86_64: W: no-documentation poco-devel.x86_64: W: no-dependency-on poco/poco-libs/libpoco poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/styles.css poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/prettify.css poco-foundation.x86_64: W: no-documentation poco-mysql.x86_64: W: no-documentation poco-net.x86_64: W: no-documentation poco-netssl.x86_64: W: no-documentation poco-odbc.x86_64: W: no-documentation poco-sqlite.x86_64: W: no-documentation poco-testing.x86_64: W: no-documentation poco-util.x86_64: W: no-documentation poco-xml.x86_64: W: no-documentation poco-zip.x86_64: W: no-documentation 16 packages and 0 specfiles checked; 2 errors, 15 warnings. $
Hello Maxim, thanks for updating the package. At the same time I've been doing a detailed review of your previous version (1.3.5-2). I found several problems. Here are my results so far. rpmlint output: poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/styles.css poco-doc.x86_64: W: wrong-file-end-of-line-encoding /usr/share/doc/poco-doc-1.3.5/css/prettify.css poco-mysql.x86_64: W: no-documentation poco-odbc.x86_64: W: no-documentation poco-testing.x86_64: W: no-documentation poco-zip.x86_64: W: no-documentation poco.src: W: no-buildroot-tag 9 packages and 0 specfiles checked; 0 errors, 7 warnings. All these rpmlint warning can be ignored. Though the BuildRoot tag might be useful to add if you are going to have an EL-5 branch too. Let's see if the package meets Review Guidelines ( http://fedoraproject.org/wiki/Packaging:ReviewGuidelines ): OK rpmlint OK name meets Package Naming Guidelines (Personally I preferred the original name of "poco-testing", i.e. "poco-debug", because it has a precedent in "kernel-debug", but it's not a strong requirement. I'll leave the decision to you.) OK the spec file name matches the base package name Package must meet the Packaging Guidelines: BAD versioning: please use %{?dist} (not %{dist}) in the Release field. OK licensing ("Boost" is an approved license) BAD pre-built binaries found in the source tarball: Crypto/include/Poco/.DS_Store Crypto/include/Poco/._.DS_Store These must be removed during the %prep step. You should also ask upstream to remove them from future releases. (See http://fedoraproject.org/wiki/Packaging/Guidelines#No_inclusion_of_pre-built_binaries_or_libraries ) OK spec is legible OK architecture support OK filesystem layout (FHS) OK changelog entries OK tags OK buildroot cleaned for %install OK buildroot cleaned in %clean OK Requires OK no PreReq OK no file dependencies outside allowed dirs OK BuildRequires OK Summary and Description (English, descriptive, no trademarks) OK encoding (ASCII) OK documentation (main documentation is split into *-doc) BAD compilation does not respect Fedora's compiler flags (%{optflags}) While observing the running build process, I could not see any mention of FORTIFY_SOURCE, stack-protector, etc. on the command lines of g++ invocations. OK debuginfo OK devel package OK subpackages require base package with fully versioned dep OK ldconfig OK no static lib BAD duplication of system libraries: poco-1.3.5-all/Foundation/src/ contains internal copies of zlib and pcre libraries. poco-1.3.5-all/XML/src/ contains a copy of expat. poco-1.3.5-all/Data/SQLite/src/ contains a copy of sqlite (the whole in a single 3.5 MB source *.c file!) The package must be modified to use system libraries. (See http://fedoraproject.org/wiki/Packaging/Guidelines#Duplication_of_system_libraries ) OK no rpath N/A config files N/A initscripts N/A desktop files OK macro usage N/A locale files N/A timestamps OK parallel make OK scriptlets N/A conditional deps OK file and dir ownership OK file permissions N/A users and groups N/A web apps OK no conflicts OK no kernel modules OK no files in /srv OK no bundling of multiple projects (combining the -all and -doc tarballs is acceptable, they're a single project) N/A bug links and comments for patches OK no epoch N/A no symlink hackery OK License field matches actual license (Boost) OK LICENSE file is packaged as %doc OK spec file is written in American English (AFAICT) OK sources match upstream source. sha256sums: 88bce8880bd380c2ca600cf170388eb0180b0c46fe500240efecc05bc62c618a poco-1.3.5-all.tar.bz2 48465ad08c9114f0fa16835344e775714fdebe93a564f4ce9b9843454aa48225 poco-1.3.5-doc.tar.gz OK package builds in Koji on all primary architectures OK package contains code or permissible content OK large doc in -doc subpackage OK missing doc should not affect runtime OK headers are in -devel N/A no pkgconfig files OK .so files in -devel OK no .la files
What's the rationale for breaking the package into so many subpackages in revision 3? I understood that a separate poco-mysql made sense because that part is of poco is not always used and it drags in a mysql dependency. But what benefit does it bring to split -xml, -net, -netssl, -zip? Isn't it taking it a bit too far?
BTW, it seems that Debian maintainers noticed the bundled zlib copy in poco too and patched it away already. See the patch 30_use-system-zlib.dpatch from http://ftp.de.debian.org/debian/pool/main/p/poco/poco_1.3.5-1.diff.gz
Hello, this update fixes some of the problems that you mentioned. With regard to subpackages - decided to go the same route as in Debian's POCO and Fedora's Boost. Please make it clear if you insist on merging subpackages and which of them. Spec URL: http://crow-designer.sourceforge.net/temp/4/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/4/poco-1.3.5-4.fc11.src.rpm CHANGES ======= - The name of 'poco-testing' subpackage was reverted to 'poco-debug'. - The "Release" field was fixed to use "%{?dist}". - The ".*DS_Store" files removal was moved to the "prep" section. - Fedora compilation flags (%{optflags}) are now injected into the "configure" script.
(In reply to comment #16) > With regard to subpackages - decided to go the same route as in Debian's POCO > and Fedora's Boost. Please make it clear if you insist on merging subpackages > and which of them. I will not insist on merging the subpackages back. The similarity with Boost packaging is a good enough reason to leave them separated. Thanks for the explanation.
Spec URL: http://crow-designer.sourceforge.net/temp/5/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/5/poco-1.3.5-5.fc11.src.rpm CHANGES ======= - A patch "poco-1.3.5-syslibs.patch" was added. The build process now does not use bundled versions of the system libraries (zlib, pcre, sqlite and expat).
Hello Maxim. Very good, you fixed the major issue I had with the package. I suggest you to add a comment before the Patch0:... line to explain why the patch is needed (Also see http://fedoraproject.org/wiki/Packaging/Guidelines#All_patches_should_have_an_upstream_bug_link_or_comment ). Have you contacted upstream developers to discuss their bundling of libraries? It would be nice if they could stop doing that or at least make it optional with a ./configure settings. Though it's not strictly necessary, it is a good idea to keep a live relationship with upstream. Your main package "poco" depends on "poco-devel". This is unusual and I don't recommend it. You said Boost packaging was your inspiration when you separated the package into several subpackages, but Boost does not do this.
(In reply to comment #19) > Have you contacted upstream developers to discuss their bundling of libraries? > It would be nice if they could stop doing that or at least make it optional > with a ./configure settings. > Though it's not strictly necessary, it is a good idea to keep a live > relationship with upstream. If I recall correctly, somebody already asked POCO developers to do something about bundled libraries problem and there was a refusal. However, a good patch that adds an appropriate option to the "configure" script may change their minds. Such a patch may be written at some point in the future. > Your main package "poco" depends on "poco-devel". This is unusual and I don't > recommend it. You said Boost packaging was your inspiration when you separated > the package into several subpackages, but Boost does not do this. Unusual, but not without benefits. "poco" is a metapackage that helps the developer to install the whole toolkit at once, including headers ("poco-devel") and documentation ("poco-doc"). If this layout is not welcome, it may be reorganized: instead of "poco", "poco-devel" package becomes a new toplevel, not depending on "poco-doc". The name of the spec file will have to be "poco-devel.spec" in this case. The boost toplevel package depends only on binary subpackages which has little meaning to either developers or users.
(In reply to comment #20) > Unusual, but not without benefits. "poco" is a metapackage that helps the > developer to install the whole toolkit at once, including headers > ("poco-devel") and documentation ("poco-doc"). I believe that developers are used to installing a *-devel package and a *-doc package when they need them and therefore such a convenience package is of limited usefulness. So I'd drop the "poco" package completely and have the spec file produce only subpackages. The %description paragraph would have to be copied to every subpackage. And the MANIFEST file should not be packaged at all, it's of no use. On the other hand, such a convenience package does not break any guidelines, so if you really like to have it, I won't push for the change. > If this layout is not welcome, > it may be reorganized: instead of "poco", "poco-devel" package becomes a new > toplevel, not depending on "poco-doc". The name of the spec file will have to > be "poco-devel.spec" in this case. Please no. That would be ugly. > The boost toplevel package depends only on binary subpackages which has little > meaning to either developers or users. Agreed. Please take a look at "rpm -q --changelog poco". You'll see macros got expanded there. Use %% to prevent macro expansion in the changelog. Or just avoid the % character completely.
(In reply to comment #21) > > Please no. That would be ugly. > Yes, ugly, but you said that you'd drop the "poco" toplevel package - how to do this in a more pleasing way? Can you give an example (or reference)? Why the full description must be in every subpackage, may be just "poco-devel" will suffice?
(In reply to comment #22) > Yes, ugly, but you said that you'd drop the "poco" toplevel package - how to > do this in a more pleasing way? Can you give an example (or reference)? Something like this will prevent the generation of the toplevel binary package: --- poco.spec.orig 2009-11-11 13:50:27.000000000 +0100 +++ poco.spec 2009-11-12 16:00:44.865027561 +0100 @@ -21,9 +21,6 @@ BuildRequires: sqlite-devel BuildRequires: expat-devel -Requires: poco-devel = %{version}-%{release} -Requires: poco-doc = %{version}-%{release} - %description The POCO C++ Libraries (POCO stands for POrtable COmponents) are open source C++ class libraries that simplify and accelerate the @@ -95,10 +92,6 @@ %clean rm -rf $RPM_BUILD_ROOT -%files -%defattr(-, root, root, -) -%doc MANIFEST - %package foundation Summary: The Foundation POCO component Group: System Environment/Libraries > Why the full description must be in every subpackage, may be just "poco-devel" > will suffice? OK, it will be sufficient.
Spec URL: http://crow-designer.sourceforge.net/temp/6/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/6/poco-1.3.5-6.fc11.src.rpm CHANGES ======= - The generation of the "poco" metapackage is now suppressed. - A comment for the patch was added. - The usage of %% symbol in the %%changelog section was fixed.
Maxim, even though there is no top-level binary package now, the Group, Summary and %description fields should still be filled. They will be used for the source package itself (you can see them with rpm -qip poco*.src.rpm).
SPEC URL: http://crow-designer.sourceforge.net/temp/7/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/7/poco-1.3.5-7.fc11.src.rpm CHANGES ======= - A removal of the "Foundation/src/MSG00001.bin" binary file was added to the "%%prep" section. - Values for the top "Summary", "Group" and "%%description" were restored. - A "BuildRoot" tag was added.
Almost perfect. I noticed that the poco-debuginfo package does not contain useful debuginfo for the non-debug versions of the libraries. It is because poco's build process strips the "release" binaries. Please fix it. An easy way is to redefine the STRIP variable to a command with no effect, like this: make %{?_smp_mflags} STRIP=/bin/true (See https://fedoraproject.org/wiki/Packaging:Debuginfo for more information about debuginfo packages.) This will be the last change necessary to pass the review. So after you've done this change, consider the package APPROVED. I'm reassigning this BZ back to Peter who will sponsor you.
Thankee, Michal! SPEC URL: http://crow-designer.sourceforge.net/temp/8/poco.spec SRPM URL: http://crow-designer.sourceforge.net/temp/8/poco-1.3.5-8.fc12.src.rpm CHANGES ======= - The "make" invocation command in the %%build section was modified to skip premature symbol stripping from retail libraries.
Peter, you assigned the bug back to me without comment. Is there something you want me to do? I approve Maxim's package. FAS shows that Maxim is not sponsored into the 'packager' group yet. I am not a sponsor. You said you'd sponsor Maxim when the review is done. Please proceed. Thanks, Michal
Ah, sorry Michal - I completely missed your comment, regarding your reassignment of this ticket. I thought it was reassigned to me by mistake.
Unblocking FE-NEEDSPONSOR - I just sponsored Maxim.
New Package CVS Request ======================= Package Name: poco Short Description: C++ class libraries for network-centric applications Owners: udushlivy Branches: F-11 F-12 EL-5 InitialCC:
cvs done.
poco-1.3.5-8.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/poco-1.3.5-8.fc12
poco-1.3.5-8.fc12 has been pushed to the Fedora 12 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with su -c 'yum --enablerepo=updates-testing update poco'. You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F12/FEDORA-2009-12333
poco-1.3.5-8.fc12 has been pushed to the Fedora 12 stable repository. If problems still persist, please make note of it in this bug report.
Hi Maxim, The latest poco package is wrong in version and release. See http://fedoraproject.org/wiki/PackageNamingGuidelines#Post-Release_packages Can you correct it to the right way. Regard, Chen Lei