Bug 1090499 - Review Request: netresolve - Generic name resolution library
Summary: Review Request: netresolve - Generic name resolution library
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Jiri Popelka
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
: 1099435 (view as bug list)
Depends On:
Blocks: dualstack
TreeView+ depends on / blocked
 
Reported: 2014-04-23 13:18 UTC by Pavel Šimerda (pavlix)
Modified: 2016-01-05 11:42 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-01-05 11:42:21 UTC
jpopelka: fedora-review+


Attachments (Terms of Use)
changes (1.99 KB, patch)
2014-04-24 11:50 UTC, Pavel Šimerda (pavlix)
no flags Details | Diff
Build log (137.06 KB, text/plain)
2015-09-24 13:40 UTC, Tomáš Hozza
no flags Details

Description Pavel Šimerda (pavlix) 2014-04-23 13:18:36 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.1.20140422git.fc21.src.rpm

Description:
Netresolve is a package for nonblocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc.

Comment 1 Michael Schwendt 2014-04-23 17:11:25 UTC
Consider pointing the fedora-review tool at this ticket. Run "fedora-review -b 1090499". It evaluates the "SRPM URL:" and "Spec URL:" lines and performs many helpful checks you ought to be interested in.


A brief look at the package:


> URL: https://sourceware.org/netresolve/

Forbidden
You don't have permission to access /netresolve/ on this server.


> Source0: netresolve-0.0.1.tar.xz

https://fedoraproject.org/wiki/Packaging:SourceURL#Referencing_Source


> %package devel
> Summary: Development files for getdns
> Group: Development/Libraries

If you set the optional Group tag for this subpackage, why is it missing in the base package? "Group: System Environment/Libraries"
https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag


> Requires: %{name} = %{version}-%{release}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package


> Requires: pkgconfig

There are automatic pkgconfig dependencies for a long time. Query the built packages. You would only need this explicit dep for EL5. But the package does not include any .pc file, so the dependency is superfluous currently.


> %post
> /sbin/ldconfig
>
> %postun
> /sbin/ldconfig

If you don't to execute anything else, consider executing ldconfig directly instead of running it within a /bin/sh script:

  %post -p /sbin/ldconfig

  %postun -p /sbin/ldconfig


> %doc NEWS COPYING

Why not include README and TODO?

Instead, the NEWS file contents are rather useless so far. 

Btw, it declares this as "0.0.1", but if there is a 0.0.1 release, the RPM package ought not apply the pre-release snapshot versioning scheme, but apply the post-release versioning scheme:
https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning


> PKG_CHECK_MODULES([ARES], [libcares])

https://fedoraproject.org/wiki/Packaging:Guidelines#BuildRequires_based_on_pkg-config


> build.log

Output is non-verbose. One cannot see whether Fedora's %optflags are used, for example, and one cannot verify the compiler/preprocessor settings.


Is the included "tests" directory suitable for running it at build-time in the spec %check section?


> checking for ARES... yes
> checking for ub_ctx_create in -llibunbound... no

This check fails, but it linked with libunbound nevertheless. Suspicious.

Comment 2 Pavel Šimerda (pavlix) 2014-04-24 11:50:14 UTC
Created attachment 889255 [details]
changes

(In reply to Michael Schwendt from comment #1)
> > URL: https://sourceware.org/netresolve/
> 
> Forbidden
> You don't have permission to access /netresolve/ on this server.

Yes, the upstream website hasn't been launched yet but I received numerous requests to come up with a Fedora package.

> > Source0: netresolve-0.0.1.tar.xz
> 
> https://fedoraproject.org/wiki/Packaging:SourceURL#Referencing_Source

It is a git snapshot and the sourceware git doesn't offer tarballs. I already contacted sourceware maintainers about it.

> > %package devel
> > Summary: Development files for getdns
> > Group: Development/Libraries
> 
> If you set the optional Group tag for this subpackage, why is it missing in
> the base package? "Group: System Environment/Libraries"
> https://fedoraproject.org/wiki/Packaging:Guidelines#Group_tag

Added.

> > Requires: %{name} = %{version}-%{release}
> 
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Fixed.

> > Requires: pkgconfig
> 
> There are automatic pkgconfig dependencies for a long time. Query the built
> packages. You would only need this explicit dep for EL5. But the package
> does not include any .pc file, so the dependency is superfluous currently.

Removed.

> > %post
> > /sbin/ldconfig
> >
> > %postun
> > /sbin/ldconfig
> 
> If you don't to execute anything else, consider executing ldconfig directly
> instead of running it within a /bin/sh script:
> 
>   %post -p /sbin/ldconfig
> 
>   %postun -p /sbin/ldconfig

Done.

> > %doc NEWS COPYING
> 
> Why not include README and TODO?

Added.

> Instead, the NEWS file contents are rather useless so far. 

Let's get ready for the releases.

> Btw, it declares this as "0.0.1", but if there is a 0.0.1 release, the RPM
> package ought not apply the pre-release snapshot versioning scheme, but
> apply the post-release versioning scheme:

This is a 0.0.1 pre-release package, no release exists, yet.

> https://fedoraproject.org/wiki/Packaging:NamingGuidelines#Package_Versioning
> 
> 
> > PKG_CHECK_MODULES([ARES], [libcares])
> 
> https://fedoraproject.org/wiki/Packaging:
> Guidelines#BuildRequires_based_on_pkg-config

Fixed.

> > build.log
> 
> Output is non-verbose. One cannot see whether Fedora's %optflags are used,
> for example, and one cannot verify the compiler/preprocessor settings.

Fixed.

> Is the included "tests" directory suitable for running it at build-time in
> the spec %check section?

Definitely yes.

> > checking for ARES... yes
> > checking for ub_ctx_create in -llibunbound... no
> 
> This check fails, but it linked with libunbound nevertheless. Suspicious.

Fixed upstream.

https://sourceware.org/git/?p=netresolve.git;a=commitdiff;h=371bf5d950a579625d474c8526e6a4cf3688f73c

Will attach new spec and srpm later.

Comment 3 Pavel Šimerda (pavlix) 2014-05-20 10:12:01 UTC
*** Bug 1099435 has been marked as a duplicate of this bug. ***

Comment 4 Pavel Šimerda (pavlix) 2014-05-20 11:54:04 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.2.20140422git.fc21.src.rpm

Description:
Netresolve is a package for nonblocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc.

Comment 5 Christopher Meng 2014-05-21 04:32:32 UTC
Sorry, after running the building process for almost 4 hrs it still couldn't get terminated, so I can't review this package on my computer. Please find other reviewers to help you step forward.

Comment 6 Pavel Šimerda (pavlix) 2014-05-21 08:04:09 UTC
(In reply to Christopher Meng from comment #5)
> Sorry, after running the building process for almost 4 hrs it still couldn't
> get terminated, so I can't review this package on my computer. Please find
> other reviewers to help you step forward.

No problem. I'm just curious about the build process as the package is very small with few dependencies, so this is the last package I would expect to take a long compilation time.

Comment 7 Christopher Meng 2014-05-21 08:15:06 UTC
Yes it's a bug IMO. I haven't saved the build log so I don't know what happened still, but if you want me to help I can run the build process again.

Comment 8 Pavel Šimerda (pavlix) 2014-05-21 11:12:08 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.3.20140422git.fc21.src.rpm

Description:
Netresolve is a package for nonblocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc.

Comment 9 Pavel Šimerda (pavlix) 2014-05-21 11:12:11 UTC
This package built on koji:  http://koji.fedoraproject.org/koji/taskinfo?taskID=6870289

Comment 10 Pavel Šimerda (pavlix) 2014-05-21 11:14:02 UTC
I just had to remove the call to tests. I don't know exactly why the test freezes during the build.

Comment 11 Tomáš Hozza 2015-01-12 11:30:32 UTC
Please update the SRPM with to the latest git HEAD.

Comment 12 Pavel Šimerda (pavlix) 2015-09-23 10:22:39 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.5.20150923git.fc24.src.rpm

Description:
Netresolve is a package for non-blocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc as well as a testbed for future glibc improvements.

Comment 13 Tomáš Hozza 2015-09-24 13:40:25 UTC
Created attachment 1076550 [details]
Build log

Mock build fails. Some tests seem to be failing. Please resolve the issue.

Comment 14 Tomáš Hozza 2015-09-24 13:40:52 UTC
I tried also COPR:
http://copr-fe.cloud.fedoraproject.org/coprs/thozza/netresolve/build/118060/

Comment 15 Upstream Release Monitoring 2015-09-30 14:45:22 UTC
pavlix's scratch build of netresolve-0.0.1-0.5.20150930git.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11278160

Comment 16 Pavel Šimerda (pavlix) 2015-10-01 05:54:36 UTC
For completeness, my rawhide build ran without problem including the tests.

(In reply to Tomas Hozza from comment #14)
> I tried also COPR:
> http://copr-fe.cloud.fedoraproject.org/coprs/thozza/netresolve/build/118060/

The coper build shows that the tests are sensitive to the existence of 'localhost4' in '/etc/hosts'. I will fix that.

(In reply to Tomas Hozza from comment #13)
> Created attachment 1076550 [details]
> Build log
> 
> Mock build fails. Some tests seem to be failing. Please resolve the issue.

From the build log:

+ diff -u /dev/fd/63 ./tests/data/localhost
++ libtool execute valgrind --leak-check=full --error-exitcode=1 ./netresolve --backends hosts --node localhost

--- /dev/fd/63	2015-09-24 15:09:48.096146394 +0200
+++ ./tests/data/localhost	2015-09-17 22:47:15.000000000 +0200
@@ -1,5 +1,6 @@
 response netresolve 0.0.1
 name localhost
+ip ::1 any any 0 0 0 0
 ip 127.0.0.1 any any 0 0 0 0
 secure
 
The diff above shows that '/etc/hosts' lacks IPv6 loopback address for 'localhost' name. That is IMO a bug in the build environment rather then the test. Some of the tests depend on the build environment but in most cases this is intentional. The objective is to also test that the system configuration allows netresolve to provide correct results.

Comment 17 Jiri Popelka 2015-10-09 16:16:03 UTC
%define snapshot_suffix .20150923git

https://fedoraproject.org/wiki/Packaging:Guidelines#.25global_preferred_over_.25define

Comment 18 Upstream Release Monitoring 2015-10-12 09:07:48 UTC
pavlix's scratch build of netresolve-0.0.1-0.5.20150930git.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11412615

Comment 19 Pavel Šimerda (pavlix) 2015-10-15 21:31:27 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.6.20151015git.fc24.src.rpm

Description:
Netresolve is a package for non-blocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc as well as a testbed for future glibc improvements.

Comment 20 Jiri Popelka 2015-10-16 13:14:09 UTC
Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated

Issues:
=======
[!]: ldconfig called in %post and %postun if required.
  Note: /sbin/ldconfig not called in netresolve-core, netresolve-compat,
  netresolve-backends-compat, netresolve-backends-aresdns, netresolve-
  backends-avahi, netresolve-backends-ubdns
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

[!]: 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.
  Note: License file COPYING is marked as %doc instead of %license
  See:
  http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

[!]: License field in the package spec file matches the actual license.
     Note: baskends/asyncns.c is LGPLv2+ licensed

[!]: License file installed when any subpackage combination is installed.
     Note: you need %license COPYING in each %files section
     (except the main package and -tools/-compat subpackages AFAICT)

[!]: Fully versioned dependency in subpackages if applicable.
     Note: It'd better to replace for example
     - Requires: netresolve-core
     + Requires: netresolve-core%{?_isa} = %{version}-%{release}
     (the same for all subpackages)

===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Header files in -devel subpackage, if present.
[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:
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[!]: License field in the package spec file matches the actual license.
     baskends/asyncns.c is LGPLv2+ licensed
[!]: License file installed when any subpackage combination is installed.
     you need %license COPYING in each %files section
     (except the main package and -tools/-compat subpackages AFAICT)
[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 without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[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.
[x]: Requires correct, justified where necessary.
[x]: Spec file is legible and written in American English.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[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 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:
[x]: Reviewer should test that the package builds in mock.
[x]: Final provides and requires are sane.
[!]: Fully versioned dependency in subpackages if applicable.
     It'd better to replace for example
     - Requires: netresolve-core
     + Requires: netresolve-core%{?_isa} = %{version}-%{release}
     (for all subpackages)
[x]: Latest version is packaged.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[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]: Uses parallel make %{?_smp_mflags} macro.
[x]: Spec use %global instead of %define unless justified.

Comment 21 Pavel Šimerda (pavlix) 2015-10-19 09:19:55 UTC
(In reply to Jiri Popelka from comment #20)
> [!]: ldconfig called in %post and %postun if required.
>   Note: /sbin/ldconfig not called in netresolve-core, netresolve-compat,
>   netresolve-backends-compat, netresolve-backends-aresdns, netresolve-
>   backends-avahi, netresolve-backends-ubdns
>   See: http://fedoraproject.org/wiki/Packaging/Guidelines#Shared_Libraries

OK.

> [!]: 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.
>   Note: License file COPYING is marked as %doc instead of %license
>   See:
>   http://fedoraproject.org/wiki/Packaging/LicensingGuidelines#License_Text

OK.

> [!]: License field in the package spec file matches the actual license.
>      Note: baskends/asyncns.c is LGPLv2+ licensed

TODO: Will fix upstream and import the new changes.

> [!]: License file installed when any subpackage combination is installed.
>      Note: you need %license COPYING in each %files section
>      (except the main package and -tools/-compat subpackages AFAICT)

TODO: Will reconsider the dependencies and fix it.

> [!]: Fully versioned dependency in subpackages if applicable.
>      Note: It'd better to replace for example
>      - Requires: netresolve-core
>      + Requires: netresolve-core%{?_isa} = %{version}-%{release}
>      (the same for all subpackages)

OK.

Comment 22 Upstream Release Monitoring 2015-11-06 11:29:54 UTC
pavlix's scratch build of netresolve-0.0.1-0.6.20151015git.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11725080

Comment 23 Upstream Release Monitoring 2015-11-06 16:28:26 UTC
pavlix's scratch build of netresolve-0.0.1-0.6.20151015git.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11728303

Comment 24 Upstream Release Monitoring 2015-11-09 08:37:44 UTC
pavlix's scratch build of netresolve-0.0.1-0.6.20151015git.fc24.src.rpm for rawhide failed http://koji.fedoraproject.org/koji/taskinfo?taskID=11753550

Comment 25 Upstream Release Monitoring 2015-11-09 09:25:53 UTC
pavlix's scratch build of netresolve-0.0.1-0.6.20151015git.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11754048

Comment 26 Pavel Šimerda (pavlix) 2015-11-11 13:03:09 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.7.20151111git.fc24.src.rpm

Description:
Netresolve is a package for non-blocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc as well as a testbed for future glibc improvements.

Comment 27 Upstream Release Monitoring 2015-11-11 13:14:36 UTC
pavlix's scratch build of netresolve-0.0.1-0.7.20151111git.fc24.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=11792116

Comment 28 Jiri Popelka 2015-11-11 13:50:15 UTC
Ok, all my objections seem to be resolved now.

But I have a new one ;-)
The dependencies doesn't look correct to me.
The main package require all subpackages and each subpackage requires main package.

I guess you wanted to use
Requires: %{name}-core%{?_isa} = %{version}-%{release}
instead of
Requires: %{name}%{?_isa} = %{version}-%{release}
?

Comment 29 Pavel Šimerda (pavlix) 2015-11-24 14:16:51 UTC
Spec URL: http://pavlix.fedorapeople.org//netresolve.spec
SRPM URL: http://pavlix.fedorapeople.org//netresolve-0.0.1-0.7.20151111git.fc24.src.rpm

Description:
Netresolve is a package for non-blocking network name resolution via backends
intended as a replacement for name service switch based name resolution in
glibc as well as a testbed for future glibc improvements.

Comment 30 Jiri Popelka 2015-11-24 14:32:02 UTC
Looks good to me, package approved.


Note You need to log in before you can comment on or make changes to this bug.