Bug 1692166

Summary: Review Request: gnatcoll-db - The GNAT Components Collection – database packages
Product: [Fedora] Fedora Reporter: Björn Persson <bjorn>
Component: Package ReviewAssignee: Richard W.M. Jones <rjones>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: 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
Spec URL: https://www.rombobjörn.se/packages/gnatcoll-db-2018-1/gnatcoll-db.spec
SRPM URL: https://www.rombobjörn.se/packages/gnatcoll-db-2018-1/gnatcoll-db-2018-1.fc30.src.rpm

This is the third and last part of Gnatcoll – the database module. It provides an object-oriented, high-level interface to SQL queries with support for PostgreSQL and SQLite. It also contains the Xref component and Gnatinspect, which build and query a database of software cross-reference information.

Fedora Account System Username: rombobeorn

Comment 1 Björn Persson 2019-04-03 20:23:42 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

Comment 2 Richard W.M. Jones 2019-05-25 04:59:54 UTC
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.

Comment 3 Richard W.M. Jones 2019-05-25 05:06:59 UTC
FYI here's the fedora-review bug: https://bugzilla.redhat.com/show_bug.cgi?id=1713842

Comment 4 Richard W.M. Jones 2019-05-25 05:19:02 UTC
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

Comment 5 Richard W.M. Jones 2019-05-25 05:23:39 UTC
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}

Comment 6 Richard W.M. Jones 2019-05-25 05:26:23 UTC
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.

Comment 7 Björn Persson 2019-05-25 16:50:24 UTC
(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.

Comment 8 Björn Persson 2019-05-26 14:08:19 UTC
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

Comment 9 Richard W.M. Jones 2019-05-26 14:34:24 UTC
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.

Comment 10 Richard W.M. Jones 2019-05-26 14:47:19 UTC
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.

Comment 11 Björn Persson 2019-05-28 16:42:40 UTC
(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).

Comment 12 Richard W.M. Jones 2019-05-28 17:04:29 UTC
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
------------

Comment 13 Gwyn Ciesla 2019-05-29 13:10:36 UTC
(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/gnatcoll-db

Comment 14 Fedora Update System 2019-05-29 16:19:54 UTC
FEDORA-2019-cfeb86a4bf has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-cfeb86a4bf

Comment 15 Björn Persson 2019-05-29 16:51:11 UTC
Thanks Richard for the review.

Comment 16 Fedora Update System 2019-05-30 13:58:11 UTC
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

Comment 17 Fedora Update System 2019-06-11 02:41:22 UTC
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.