Bug 1692166
Summary: | Review Request: gnatcoll-db - The GNAT Components Collection – database packages | ||
---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Björn Persson <bjorn> |
Component: | Package Review | Assignee: | Richard W.M. Jones <rjones> |
Status: | CLOSED CURRENTRELEASE | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
Severity: | medium | Docs Contact: | |
Priority: | medium | ||
Version: | rawhide | CC: | package-review, pzhukov, rjones |
Target Milestone: | --- | Flags: | rjones:
fedora-review+
|
Target Release: | --- | ||
Hardware: | All | ||
OS: | Linux | ||
Whiteboard: | |||
Fixed In Version: | Doc Type: | If docs needed, set a value | |
Doc Text: | Story Points: | --- | |
Clone Of: | Environment: | ||
Last Closed: | 2019-05-29 16:51:11 UTC | Type: | --- |
Regression: | --- | Mount Type: | --- |
Documentation: | --- | CRM: | |
Verified Versions: | Category: | --- | |
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
Cloudforms Team: | --- | Target Upstream Version: | |
Embargoed: | |||
Bug Depends On: | 1689552 | ||
Bug Blocks: |
Description
Björn Persson
2019-03-24 20:02:00 UTC
Fixed some things similar to what came up in the review of gnatcoll-bindings: https://www.rombobjörn.se/packages/gnatcoll-db-2018-2/gnatcoll-db.spec https://www.rombobjörn.se/packages/gnatcoll-db-2018-2/gnatcoll-db-2018-2.fc30.src.rpm Your IDN causes problems for fedora-review. It tries to convert it to ASCII by basically doing ö -> o, and that works about as well as you'd expect: ERROR: 'Error [Errno socket error] [Errno -2] Name or service not known downloading https://www.rombobjrn.se/packages/gnatcoll-db-2018-2/gnatcoll-db-2018-2.fc30.src.rpm' (logs in /home/rjones/.cache/fedora-review.log) I'll try it with overriding the URLs. FYI here's the fedora-review bug: https://bugzilla.redhat.com/show_bug.cgi?id=1713842 Most obvious things from a manual review of the spec file. These seem both complex and unnecessary: # This readme file may be of some value to developers: mkdir --parents %{buildroot}%{_docdir}/gnatcoll/xref cp --preserve=timestamps xref/README.md \ --target-directory=%{buildroot}%{_docdir}/gnatcoll/xref # Install the license in a directory named after the source package. mkdir --parents %{buildroot}%{_licensedir}/%{name} cp --preserve=timestamps COPYING3 \ --target-directory=%{buildroot}%{_licensedir}/%{name} I would replace them with (in %files): %doc xref/README.md %license COPYING3 and get rid of %{_docdir}/gnatcoll in %files devel too. The %license change is required. The %doc change probably not, but I would recommend it. I don't know if the following is needed (because I believe that rpaths are already checked by RPM), but I guess it doesn't do any harm. Most likely it can be deleted: %check %{_rpmconfigdir}/check-rpaths rpmlint says: gnatcoll-sql.x86_64: I: enchant-dictionary-not-found sv - fine, ignore gnatcoll-sql.x86_64: W: no-documentation gnatcoll-sqlite.x86_64: W: no-documentation gnatcoll-postgres.x86_64: W: no-documentation gnatcoll-xref.x86_64: W: no-documentation - adding %doc xref/README.md would fix all these gnatcoll-xref.x86_64: W: spelling-error %description -l en_US ali -> ail, Ali, tali gnatcoll-xref.x86_64: W: spelling-error %description -l en_US gli -> glee, glib, Eli - fine, ignore gnatcoll-xref.x86_64: W: no-manual-page-for-binary gnatinspect gnatcoll-db-devel.x86_64: W: no-manual-page-for-binary gnatcoll_all2ada gnatcoll-db-devel.x86_64: W: no-manual-page-for-binary gnatcoll_db2ada - if one is available upstream it should be added, or you could write them; however it's not a review blocker gnatcoll-db-devel.x86_64: W: no-dependency-on gnatcoll-db/gnatcoll-db-libs/libgnatcoll-db - however there is a correct dependency on the base package (gnatcoll-sql) and other required packages, so this can be ignored gnatcoll-db.src:83: W: unversioned-explicit-provides gnatinspect gnatcoll-db.src:108: W: unversioned-explicit-provides gnatcoll_db2ada gnatcoll-db.src:108: W: unversioned-explicit-provides gnatcoll_all2ada - this is a bug; the Provides lines should all have versions, ie: Provides: gnatinspect = %{version}-%{release} ... Provides: gnatcoll_db2ada = %{version}-%{release} Provides: gnatcoll_all2ada = %{version}-%{release} Fedora-review doesn't point to any other major issues, but please fix the above issues and post a new package spec/src.rpm and I will run it and do the final review. (In reply to Richard W.M. Jones from comment #4) > Most obvious things from a manual review of the spec file. These seem both > complex and unnecessary: > > # This readme file may be of some value to developers: > mkdir --parents %{buildroot}%{_docdir}/gnatcoll/xref > cp --preserve=timestamps xref/README.md \ > --target-directory=%{buildroot}%{_docdir}/gnatcoll/xref There are three reasons for this: 1: It keeps all the Gnatcoll documentation together under /usr/share/doc/gnatcoll instead of relegating some files to /usr/share/doc/gnatcoll-bindings-devel and /usr/share/doc/gnatcoll-db-devel. 2: Without the subdirectory named "xref" nothing in the pathname indicates that this readme file is specific to the xref component. 3: If upstream writes something useful in the other readme files in a future release, then subdirectories will definitely be necessary as they're all named README.md. gnatcoll-bindings-devel includes three different README.md in separate subdirectories. > %license COPYING3 That would make it /usr/share/licenses/gnatcoll-sql/COPYING3. That seems inappropriate for a license that applies to all the components of gnatcoll-db. > I don't know if the following is needed (because I believe that rpaths are > already > checked by RPM), but I guess it doesn't do any harm. Most likely it can be > deleted: > > %check > %{_rpmconfigdir}/check-rpaths As far as I understand this is not done automatically yet, but there is a proposal to do so: https://pagure.io/packaging-committee/issue/886 We have an Ada-specific policy to run check-rpaths because the GNAT tools insert runpaths by default and the option to disable this didn't always work in the past. We should be able to relax this policy after the FPC's proposal gets implemented. (In reply to Richard W.M. Jones from comment #5) > gnatcoll-xref.x86_64: W: no-manual-page-for-binary gnatinspect > gnatcoll-db-devel.x86_64: W: no-manual-page-for-binary gnatcoll_all2ada > gnatcoll-db-devel.x86_64: W: no-manual-page-for-binary gnatcoll_db2ada > > - if one is available upstream it should be added, or you could write them; > however > it's not a review blocker In gnatcoll-doc (built from the source package gnatcoll) there is a manual in HTML and PDF, but most of the information about gnatinspect and gnatcoll_db2ada has been removed in this release. The removal is probably related to the splitting of the source repository. I hope they intend to add the documentation back as separate manuals eventually. > gnatcoll-db.src:83: W: unversioned-explicit-provides gnatinspect > gnatcoll-db.src:108: W: unversioned-explicit-provides gnatcoll_db2ada > gnatcoll-db.src:108: W: unversioned-explicit-provides gnatcoll_all2ada > > - this is a bug; the Provides lines should all have versions, ie: > > Provides: gnatinspect = %{version}-%{release} > ... > Provides: gnatcoll_db2ada = %{version}-%{release} > Provides: gnatcoll_all2ada = %{version}-%{release} OK, I'll fix this. An updated package is coming. It appears that files under _licensedir don't get tagged as licenses automatically, as I thought. I've added "%license". I'm writing these links in the ACE form to see how fedora-review deals with that. https://www.xn--rombobjrn-67a.se/packages/gnatcoll-db-2018-3/gnatcoll-db.spec https://www.xn--rombobjrn-67a.se/packages/gnatcoll-db-2018-3/gnatcoll-db-2018-3.fc30.src.rpm Fedora-review says: - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ Although this is obviously an Ada package, it does contain a few C files and therefore it might require you to add: BuildRequires: gcc if those files get built as part of the process. Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. Note: No gcc, gcc-c++ or clang found in BuildRequires See: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ *** Please check and add the BR if required. ===== MUST items ===== C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. OK because all binaries are dynamically linked. [x]: ldconfig not called in %post and %postun for Fedora 28 and later. [x]: Package does not contain any libtool archives (.la) [x]: Rpath absent or only used for internal libs. [x]: Development (unversioned) .so files in -devel subpackage, if present. Generic: [ ]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. There are a few licensing things that need to be checked or fixed: GPL (with incorrect FSF address) -------------------------------- gnatcoll-db-gpl-2018-src/sqlite/gnatcoll-sql-sqlite.adb Almost every other source file lacks a license header of any kind. Neither of these are review blockers, but it would be nice if you would get upstream to fix both these things. [x]: License field in the package spec file matches the actual license. [x]: License file installed when any subpackage combination is installed. Yes, because all packages depend on gnatcoll-sql which contains the license file. [x]: Package does not own files or directories owned by other packages. Fedora-review said: Note: Dirs in package are owned also by: /usr/share/doc/gnatcoll(gnatcoll-doc, gnatcoll-bindings-devel) but I believe after discussing this with the packager this is intentional and desired. [-]: %build honors applicable compiler flags or justifies otherwise. Not really applicable for this Ada package. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. It's OK, but see my comments about license above. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [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]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. .. and Swedish! [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. Debuginfo files are created and contain data. I didn't check if they actually work with gdb. [?]: Package is not known to require an ExcludeArch tag. Didn't check but we'll find out soon enough when it's submitted to Fedora. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 1 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [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 %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [ ]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. See above. [x]: Final provides and requires are sane (see attachments). I manually checked them and they look fine. [x]: Fully versioned dependency in subpackages if applicable. Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in gnatcoll-sql , gnatcoll-sqlite , gnatcoll-postgres , gnatcoll-xref , gnatcoll-db-devel I'm not quite sure what point Fedora-review is trying to make there, but the spec file dependencies as written look fine to me. [?]: Package functions as described. [?]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [x]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. (In reply to Richard W.M. Jones from comment #10) > - If your application is a C or C++ application you must list a > BuildRequires against gcc, gcc-c++ or clang. > Note: No gcc, gcc-c++ or clang found in BuildRequires gcc-gnat pulls in gcc, and wouldn't work without it. Both Ada and C sources are compiled by invoking gcc, the compiler driver, which in turn invokes the actual compiler gnat1 for Ada or cc1 for C. If the GCC packaging would be changed to make cc1 an optional component, in a "gcc-c" package perhaps, then I might need to require that package, but as it currently is, GNAT comes with C support included. I could add "BuildRequires: gcc", but it would be purely a formality. > GPL (with incorrect FSF address) > -------------------------------- > gnatcoll-db-gpl-2018-src/sqlite/gnatcoll-sql-sqlite.adb I'm trying to bring this to Adacore's attention here: https://github.com/AdaCore/gnatcoll-db/pull/11 > Almost every other source file lacks a license header of any kind. On the contrary, almost every source file contains a license header, but Licensecheck apparently doesn't recognize them. > [-]: %build honors applicable compiler flags or justifies otherwise. > > Not really applicable for this Ada package. Actually the entire value of optflags is included in GPRbuild_optflags, and is applied to both Ada and C (although some of the options make no difference for Ada). It's up to you if you want to add BR gcc or not. I noticed that there are some plain C files in the sources and if they are compiled then maybe it's better to state that dependency explicitly. Anyhow ... ------------ This package has been APPROVED for Fedora by rjones ------------ (fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/gnatcoll-db FEDORA-2019-cfeb86a4bf has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-cfeb86a4bf Thanks Richard for the review. gnatcoll-2018-2.fc30, gnatcoll-bindings-2018-3.fc30, gnatcoll-db-2018-3.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report. See https://fedoraproject.org/wiki/QA:Updates_Testing for instructions on how to install test updates. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-cfeb86a4bf gnatcoll-2018-2.fc30, gnatcoll-bindings-2018-3.fc30, gnatcoll-db-2018-3.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report. |