[Bug 225888] Merge Review: hsqldb

Mon Jan 24 10:19:55 UTC 2011

Please do not reply directly to this email. All additional
comments should be made in the comments box of this bug.

https://bugzilla.redhat.com/show_bug.cgi?id=225888

--- Comment #3 from Alexander Kurtakov <akurtako at redhat.com> 2011-01-24 05:19:52 EST ---
(In reply to comment #2)

> === REQUIRED ITEMS ===
> [!]  Rpmlint output:
> hsqldb.noarch: W: name-repeated-in-summary C Hsqldb
> * I guess make it a bit different :-)
Fixed.
> hsqldb.noarch: W: spelling-error %description -l en_US servlets -> servants,
> serviettes, serviette
> * no problem
> hsqldb.noarch: W: non-standard-group Development/Java
> * Applications/Databases I guess?
Fixed.
> hsqldb.noarch: W: non-conffile-in-etc /etc/maven/fragments/hsqldb
> * no problem
> hsqldb.noarch: E: non-readable /var/lib/hsqldb/sqltool.rc 0600L
> * I guess this is supposed to be like that to allow only hsqldb user to do
> things. A bit of verification woul be great though
Yep, you're right it's this way to prevent random user reading a password from
the file.
> hsqldb.noarch: W: non-conffile-in-etc /etc/sysconfig/hsqldb
> * Make it into %(config)
Fixed.
> hsqldb.noarch: W: no-manual-page-for-binary hsqldbRunUtil
> * Would be great to have it...
> hsqldb.noarch: W: dangerous-command-in-%post rm
> hsqldb.noarch: W: dangerous-command-in-%preun rm
> * I prefer to have dangling symlinks, but it's up to you
Done.
> hsqldb.noarch: E: postin-without-chkconfig /etc/rc.d/init.d/hsqldb
> hsqldb.noarch: E: preun-without-chkconfig /etc/rc.d/init.d/hsqldb
> hsqldb.noarch: W: service-default-enabled /etc/rc.d/init.d/hsqldb
> hsqldb.noarch: W: service-default-enabled /etc/rc.d/init.d/hsqldb
> hsqldb.noarch: W: no-reload-entry /etc/rc.d/init.d/hsqldb
> hsqldb.noarch: E: subsys-not-used /etc/rc.d/init.d/hsqldb
> 
> * Whole init system needs looking into. Perhaps even creating systemd units
> etc.
Fixed as much as my knowledge allows me.
> 
> hsqldb.src: W: strange-permission hsqldb-1.8.0-standard.cfg 0755L
> * should probably be 644
Fixed.
> 
> hsqldb.src:214: W: macro-in-comment %{_sbindir}
> hsqldb.src:214: W: macro-in-comment %{name}
> hsqldb.src:215: W: macro-in-comment %{_sbindir}
> hsqldb.src:215: W: macro-in-comment %{name}
> 
> * remove commented code, we have git history to deal with things like that now
> :-)
Fixed by escaping.
> hsqldb.src:47: W: mixed-use-of-spaces-and-tabs (spaces: line 47, tab: line 31)
> * choose one and stick with it
Fixed.
> hsqldb.src: W: invalid-url Source0: hsqldb_1_8_0_10.zip
> * I believe this should be proper URL as is in the comment above Source0
Fixed.
> hsqldb-demo.noarch: W: non-standard-group Development/Java
> * Documentation ?
Fixed.
> hsqldb-demo.noarch: W: no-documentation
> * no problem
> hsqldb-javadoc.noarch: W: non-standard-group Development/Java
> hsqldb-manual.noarch: W: non-standard-group Development/Java
> * Documentation
Fixed.
> hsqldb-javadoc.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/javadoc/hsqldb-1.8.0.10/hsqldbstylesheet.css
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelist_1_7_2.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelog_1_7_1.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelist_1_7_3.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/hypersonic_lic.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelist_1_8_0.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/readme-docauthors.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/databaseinjar.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelog_1_8_0.txt
> hsqldb-manual.noarch: W: wrong-file-end-of-line-encoding
> /usr/share/doc/hsqldb-1.8.0.10/changelog_1_7_2.txt
> Should be fixed with sed in prep
Hmm, I don't see thsi with version 1.8.1.
> 
> 5 packages and 0 specfiles checked; 4 errors, 35 warnings.
> 
> 
> [x]  Package is named according to the Package Naming Guidelines[1].
> [x]  Spec file name must match the base package name, in the format
> %{name}.spec.
> [x]  Package meets the Packaging Guidelines[2].
> [x]  Package successfully compiles and builds into binary rpms.
> [x]  Buildroot definition is not present
> [x]  Package is licensed with an open-source compatible license and meets other
> legal requirements as defined in the legal section of Packaging
> Guidelines[3,4].
> [x]  License field in the package spec file matches the actual license.
> License type: BSD
> [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.
> [!]  All independent sub-packages have license of their own
Fixed.
> Javadoc subpackage should include license as should manual subpackage.
> [x]  Spec file is legible and written in American English.
> [x]  Sources used to build the package matches the upstream source, as provided
> in the spec URL.
> MD5SUM this package    : 17410483b5b5f267aa18b7e00b65e6e0
> [!]  All build dependencies are listed in BuildRequires, except for any that
> are listed in the exceptions section of Packaging Guidelines[5].
> I see no reason to have coreutils/shadow-utils in Requires. Also if you decide
> to move symlink generation from post/postun to build phase you can remove
> Requires(post) on servlet25
Removed.
> [x]  Package must own all directories that it creates.
> [x]  Package requires other packages for directories it uses.
> [x]  Package does not contain duplicates in %files.
> [!]  Permissions on files are set properly.
> %attr(0755,hsqldb,hsqldb) %{_localstatedir}/lib/%{name}/data
> - this world readable setting seems like a unnecessary security "risk" at first
> glance.
> 
Fixed.
> Some of other custom attrs can be also removed (644 ones).
> 
> [!]  Package does NOT have a %clean section which contains rm -rf %{buildroot}
> (or $RPM_BUILD_ROOT). (not needed anymore)
Fixed.
> * can be removed
> [!]  Package consistently uses macros (no %{buildroot} and $RPM_BUILD_ROOT
> mixing)
Fixed.
> * please use one or the other (maven metadata has buildroot macro)
> [!]  Package contains code, or permissable content.
> [!]  Fully versioned dependency in subpackages, if present.
Are there problems with these 2?
> [-]  Package contains a properly installed %{name}.desktop file if it is a GUI
> application.
> [x]  Package does not own files or directories owned by other packages.
> [x]  Javadoc documentation files are generated and included in -javadoc
> subpackage
> [!]  Javadocs are placed in %{_javadocdir}/%{name} (no -%{version} symlinks)
> [x]  Packages have proper BuildRequires/Requires on jpackage-utils
> [!]  Javadoc subpackages have Require: jpackage-utils
> Please add it
Fixed.
> [!]  Package uses %global not %define
Fixed.
> [-]  If package uses tarball from VCS include comment how to re-create that
> tarball (svn export URL, git clone URL, ...)
> [x]  If source tarball includes bundled jar/class files these need to be
> removed prior to building
> "-exec rm -f {} \;" can be replaced with "-delete" though.
> [x]  All filenames in rpm packages must be valid UTF-8.
> [!]  Jar files are installed to %{_javadir}/%{name}.jar (see [6] for details)
Fixed.
> [x]  If package contains pom.xml files install it (including depmaps) even when
> building with ant
> [x]  pom files has correct add_to_maven_depmap call which resolves to the pom
> file (use "JPP." and "JPP-" correctly)
> 
> === Maven ===
> [x]  Use %{_mavenpomdir} macro for placing pom files instead of
> %{_datadir}/maven2/poms
> [-]  If package uses "-Dmaven.test.skip=true" explain why it was needed in a
> comment
> [-]  If package uses custom depmap "-Dmaven2.jpp.depmap.file=*" explain why
> it's needed in a comment
> [x]  Package uses %update_maven_depmap in %post/%postun
> [x]  Packages have Requires(post) and Requires(postun) on jpackage-utils (for
> %update_maven_depmap macro)
> 
> === Other suggestions ===
> [x]  If possible use upstream build method (maven/ant/javac)
> [x]  Avoid having BuildRequires on exact NVR unless necessary
> [x]  Package has BuildArch: noarch (if possible)
> [!]  Latest version is packaged.
> 1.8.1.3 is latest 1.8.x upstream version (there is 2.0 too)
Updated to 1.8.1.3. Version 2.x are incompatible and this will break
LibreOffice/OpenOffice.

> [x]  Reviewer should test that the package builds in mock.
> Tested on: fedora-x86_64-rawhide
> 
> 
> === Other Issues ===
> 1. I don't understand need for manually unzipping in beginning of %prep
> 2. Patches should have comments about reasons and upstreamability
> 
> 
> [1] https://fedoraproject.org/wiki/Packaging:NamingGuidelines
> [2] https://fedoraproject.org/wiki/Packaging:Guidelines
> [3] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines
> [4] https://fedoraproject.org/wiki/Licensing:Main
> [5] https://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2
> [6] https://fedoraproject.org/wiki/Packaging:Java#Filenames

As this review was pretty bad one :). I'm asking you to do a new review
pointing the parts that still need work. I tried to fix everything mentioned
but it appears to be too much for me to handle it properly.

-- 
Configure bugmail: https://bugzilla.redhat.com/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.