Bug 1881782

Spec & SRPM are also available at:
https://copr.fedorainfracloud.org/coprs/yunyings/accel-config/build/1681815/

>Spec URL: https://github.com/yunyings/accel-config/blob/master/accel-config.spec
This link leads to a syntax-highlighted HTML rendition of the file. Please use "raw file" links to the spec.

>Group:		System Environment/Base
>Group:		Development/Libraries
>Group:		System Environment/Libraries
>Group:		Unspecified
Not used in Fedora.
https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections

>URL:		https://github.com/intel/%{project_name}
>Source0:	https://github.com/intel/%{project_name}/archive/%{name}-v%{version}.tar.gz
You can simplify Source0 by using replacing the beginning with the "%{URL}" macro.

>make %{?_smp_mflags}
Use %make_build instead.

>%package -n %{name}-test
>Summary:	Unit tests for %{name}.
Hm, I don't quite get this. Would a developer using accel-config-devel ever need to run these tests manually? Because "unit tests" makes it sound like they just test if the library has been built properly - in which case, the subpackage can be scrapped in favour of just running the test suite during the RPM build, in the %check section.

(In reply to Artur Frenszek-Iwicki from comment #2)
> >Spec URL: https://github.com/yunyings/accel-config/blob/master/accel-config.spec
> This link leads to a syntax-highlighted HTML rendition of the file. Please
> use "raw file" links to the spec.
> 
> >Group:		System Environment/Base
> >Group:		Development/Libraries
> >Group:		System Environment/Libraries
> >Group:		Unspecified
> Not used in Fedora.
> https://docs.fedoraproject.org/en-US/packaging-guidelines/#_tags_and_sections
> 
> >URL:		https://github.com/intel/%{project_name}
> >Source0:	https://github.com/intel/%{project_name}/archive/%{name}-v%{version}.tar.gz
> You can simplify Source0 by using replacing the beginning with the "%{URL}"
> macro.
> 
> >make %{?_smp_mflags}
> Use %make_build instead.
> 
Thanks a lot Artur for the review. Comments above have been fixed.

> >%package -n %{name}-test
> >Summary:	Unit tests for %{name}.
> Hm, I don't quite get this. Would a developer using accel-config-devel ever
> need to run these tests manually? Because "unit tests" makes it sound like
> they just test if the library has been built properly - in which case, the
> subpackage can be scrapped in favour of just running the test suite during
> the RPM build, in the %check section.
According to the description of this "test"(https://github.com/intel/idxd-config/tree/stable/test):
"#accel-config test
The test command is an option to test all the library code of accel-config, including set and get libaccfg functions for all components in dsa device, set large wq to exceed max total size in dsa, test the create-mdev and remove-mdev on shared wq and dedicated wq."

It's testing the basic function of the libs that accel-config provides. By running this user could know whether the libs as well as the DSA kernel driver work as expected.
So it's not checking whether the library was built properly, but functioning properly, while the -devel package is providing the libs & headers for applications to be built upon accel-config.
It makes sense to keep it, right? I will leave this unchanged for now while double checking with upstream developer about this.

Updated SPEC: https://raw.githubusercontent.com/yunyings/accel-config/master/accel-config.spec
Updated SRPM: https://github.com/yunyings/accel-config/raw/master/accel-config-2.8-1.fc32.src.rpm
Updated Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=52130166

> >%package -n %{name}-test
> >Summary:	Unit tests for %{name}.
> Hm, I don't quite get this. Would a developer using accel-config-devel ever
> need to run these tests manually? Because "unit tests" makes it sound like
> they just test if the library has been built properly - in which case, the
> subpackage can be scrapped in favour of just running the test suite during
> the RPM build, in the %check section.
According to upstream developer, most users won't use the unit test. So here I updated the spec and src.rpm again to remove the test stuff:
Updated SPEC: https://raw.githubusercontent.com/yunyings/accel-config/master/accel-config.spec
Updated SRPM: https://github.com/yunyings/accel-config/raw/master/accel-config-2.8-1.fc32.src.rpm
Updated Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=52192235

Ping.. anyone could kindly help to review this further? Thanks.

Fresh Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=53169430
I think that the artifacts from the previous one were removed.

> According to upstream developer, most users won't use the unit test. So here I updated the spec and src.rpm again to remove the test stuff:

I would add tests under %check if possible.

> BuildRequires:	autoconf
> BuildRequires:	asciidoc
> BuildRequires:	xmlto
> BuildRequires:	automake
> BuildRequires:	libtool
> BuildRequires:	pkgconfig
> BuildRequires:	pkgconfig(libkmod)
> BuildRequires:	pkgconfig(uuid)
> BuildRequires:	pkgconfig(json-c)
> BuildRequires:	pkgconfig(libudev)
> BuildRequires:	systemd

"gcc" and/or "gcc-c++"" are missing from this list.

> %description
> Utility library for configuring the accelerator subsystem.
> 
> %package -n %{name}-devel

It's not necessary to explicitly name subpackages like this if they begin with %{name}. It's enough to declare them like this:
> %package devel

The same is true for the %files block and other blocks which require package names.

> %package -n %{name}-libs
> Summary:	Configuration library for accelerator subsystem devices
> License:	LGPLv2

Are the libs useful in any way outside of the main "accel-config" package? If not, they should contain the following line:
> Requires: %{name}%{?_isa} = %{version}-%{release}

Or was it made so one can use only "accel-config-devel” and not be bothered with the main package when developing software including accel-config headers and/or linking against the SO files?

> %post -n %{name}-libs -p /sbin/ldconfig
> 
> %postun -n %{name}-libs -p /sbin/ldconfig

These ldconfig calls ought to be removed.

> %files
> %license Documentation/COPYING licenses/BSD-MIT licenses/CC0

The License field contains only "GPLv2" and "LGPLv2" for the -devel and -libs subpackages. If the package is licensed using several licenses, all of them need to be included in the License field and their breakdown indicated in a comment above that line.

Other potential issues are covered in the main review body:

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++/
- ldconfig not called in %post and %postun for Fedora 28 and later.
  Note: /sbin/ldconfig called in accel-config-libs
  See: https://fedoraproject.org/wiki/Changes/Removing_ldconfig_scriptlets


===== 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.
     Note: Using prebuilt packages
[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.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "Unknown or generated", "GPL (v2)", "GNU General Public License
     (v2) GNU Lesser General Public License", "*No copyright* Expat
     License". 89 files have unknown license. Detailed output of
     licensecheck in /home/amender/rpmbuild/SPECS/accel-config/accel-
     config/licensecheck.txt
     Review: mentioned in an earlier comment.
[x]: License file installed when any subpackage combination is installed.
[!]: Package requires other packages for directories it uses.
     Note: No known owner of /etc/accel-config
     Review: Should probably be owned by the main package. Add it to %files.
[ ]: Package must own all directories that it creates.
     Note: Directories without known owners: /etc/accel-config
     Review: As above.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.
[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[-]: Package contains desktop file if it is a GUI application.
[x]: Development files must be in a -devel package
[?]: 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.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
     Review: ExclusiveArchs correctly used.
[-]: 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.
[!]: Package complies to the Packaging Guidelines
     Review: see earlier comments and errors.
[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 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 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:
[x]: Reviewer should test that the package builds in mock.
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).
[ ]: Fully versioned dependency in subpackages if applicable.
     Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in accel-
     config-devel , accel-config-libs
     Review: see earlier comments.
[?]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Scriptlets must be sane, if used.
[-]: Sources are verified with gpgverify first in %prep if upstream
     publishes signatures.
     Note: gpgverify is not used.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[ ]: %check is present and all tests pass.
     Review: mentioned earlier.
[x]: Packages should try to preserve timestamps of original installed
     files.
[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]: The placement of pkgconfig(.pc) files are correct.
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[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: No rpmlint messages.
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Package should not use obsolete m4 macros


Rpmlint
-------
Checking: accel-config-2.8-1.fc34.x86_64.rpm
          accel-config-devel-2.8-1.fc34.x86_64.rpm
          accel-config-libs-2.8-1.fc34.x86_64.rpm
          accel-config-debuginfo-2.8-1.fc34.x86_64.rpm
          accel-config-debugsource-2.8-1.fc34.x86_64.rpm
          accel-config-2.8-1.fc32.src.rpm
accel-config.x86_64: W: non-conffile-in-etc /etc/accel-config/accel-config.conf.sample
accel-config-devel.x86_64: W: spelling-error Summary(en_US) libaccfg -> celibacy
accel-config-devel.x86_64: W: no-documentation
accel-config-libs.x86_64: W: description-shorter-than-summary
6 packages and 0 specfiles checked; 0 errors, 4 warnings.




Rpmlint (debuginfo)
-------------------
Checking: accel-config-debuginfo-2.8-1.fc34.x86_64.rpm
          accel-config-libs-debuginfo-2.8-1.fc34.x86_64.rpm
2 packages and 0 specfiles checked; 0 errors, 0 warnings.





Rpmlint (installed packages)
----------------------------
(none): E: no installed packages by name accel-config-debugsource
(none): E: no installed packages by name accel-config
(none): E: no installed packages by name accel-config-devel
(none): E: no installed packages by name accel-config-libs-debuginfo
(none): E: no installed packages by name accel-config-libs
(none): E: no installed packages by name accel-config-debuginfo
0 packages and 0 specfiles checked; 0 errors, 0 warnings.



Source checksums
----------------
https://github.com/intel/idxd-config/archive/accel-config-v2.8.tar.gz :
  CHECKSUM(SHA256) this package     : c8aa8421179b71fb728806e5a621655fab24a862f9ce982f3119b92da416949f
  CHECKSUM(SHA256) upstream package : c8aa8421179b71fb728806e5a621655fab24a862f9ce982f3119b92da416949f


Requires
--------
accel-config (rpmlib, GLIBC filtered):
    accel-config-libs(x86-64)
    libaccel-config.so.1()(64bit)
    libaccel-config.so.1(LIBACCFG_1)(64bit)
    libaccel-config.so.1(LIBACCFG_3)(64bit)
    libaccel-config.so.1(LIBACCFG_4)(64bit)
    libaccel-config.so.1(LIBACCFG_6)(64bit)
    libc.so.6()(64bit)
    libjson-c.so.5()(64bit)
    libuuid.so.1()(64bit)
    libuuid.so.1(UUID_1.0)(64bit)
    rtld(GNU_HASH)

accel-config-devel (rpmlib, GLIBC filtered):
    /usr/bin/pkg-config
    accel-config-libs(x86-64)
    libaccel-config.so.1()(64bit)

accel-config-libs (rpmlib, GLIBC filtered):
    /sbin/ldconfig
    libc.so.6()(64bit)
    libkmod.so.2()(64bit)
    libudev.so.1()(64bit)
    libuuid.so.1()(64bit)
    libuuid.so.1(UUID_1.0)(64bit)
    rtld(GNU_HASH)

accel-config-debuginfo (rpmlib, GLIBC filtered):

accel-config-debugsource (rpmlib, GLIBC filtered):



Provides
--------
accel-config:
    accel-config
    accel-config(x86-64)

accel-config-devel:
    accel-config-devel
    accel-config-devel(x86-64)
    pkgconfig(libaccel-config)

accel-config-libs:
    accel-config-libs
    accel-config-libs(x86-64)
    libaccel-config.so.1()(64bit)
    libaccel-config.so.1(LIBACCFG_1)(64bit)
    libaccel-config.so.1(LIBACCFG_3)(64bit)
    libaccel-config.so.1(LIBACCFG_4)(64bit)
    libaccel-config.so.1(LIBACCFG_5)(64bit)
    libaccel-config.so.1(LIBACCFG_6)(64bit)

accel-config-debuginfo:
    accel-config-debuginfo
    accel-config-debuginfo(x86-64)
    debuginfo(build-id)

accel-config-debugsource:
    accel-config-debugsource
    accel-config-debugsource(x86-64)

Thanks a lot for the review. I will try to fix the identified issues.

Hi Andy, sorry for the delay of update.

I've updated spec & src.rpm to fix the issues you mentioned above:
Updated SPEC: https://raw.githubusercontent.com/yunyings/accel-config/master/accel-config.spec
Updated SRPM: https://github.com/yunyings/accel-config/raw/master/accel-config-2.8-1.fc32.src.rpm
Updated Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=53740714

Could you help to review again? Thank you.

> # The entire source code is under GPLv2 except for accel-config
> # library which is mostly LGPLv2, ccan/list which is BSD-MIT and
> # the rest of ccan which is CC0.
> License:	GPLv2 and LGPLv2 and BSD-MIT and CC0

Thanks for the extra comments. However, "BSD-MIT" is not a valid license tag.

I had a look at the BSD-MIT license file again and it's the Modern Style MIT with sublicense: https://fedoraproject.org/wiki/Licensing:MIT?rd=Licensing/MIT#Modern_Style_with_sublicense

So the License tag should look like this:
> License:	GPLv2 and LGPLv2 and MIT and CC0

> Summary:	Configuration library for accelerator subsystem devices
> # All source code of configuration library is LGPLv2, except
> # ccan/list which is BSD-MIT and the rest of ccan/ which is CC0.
> License:	LGPLv2 and BSD-MIT and CC0

Same here, it should be:
> License:	GPLv2 and LGPLv2 and MIT and CC0

The rest looks okay. Please, fix that on package import. Approved!

Thanks a lot for the review and approval! 
I was not quite sure about the short name of BSD-MIT but should have digged more.. will fix the MIT this on package import.

Package has been approved for over 2 weeks, but still the new pakcage repo is not set up yet.. Anyone knows why?

Looking at releng pagure, I don't see a request for the repo to be created: https://pagure.io/releng/fedora-scm-requests/issues?search_pattern=accel-config

You need to *manually* submit a request using "fedpkg request-repo".

Indeed I forgot to request.. Thanks for the guidance Artur.
Just requested for this package: https://pagure.io/releng/fedora-scm-requests/issue/30250

(fedscm-admin):  The Pagure repository was created at https://src.fedoraproject.org/rpms/accel-config