https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Bug ID: 2166288 Summary: Review Request: libigloo - C framework from Icecast Product: Fedora Version: rawhide Hardware: All OS: Linux Status: NEW Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: ppisar@redhat.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org Target Milestone: --- Classification: Fedora
Spec URL: https://ppisar.fedorapeople.org/libigloo/libigloo.spec SRPM URL: https://ppisar.fedorapeople.org/libigloo/libigloo-0.9.2-1.fc38.src.rpm Description: This is a collection of functions used in Icecast project. Namely: deprecation warnings, string conversion functions, digests, HMAC, PRNG, UUID, error codes, logging, lists, locking, objects with reference counting, Test Any Protocol, clock. The cryptographical features are implemented with RHash library.
Fedora Account System Username: ppisar
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #1 from Jakub Kadlčík jkadlcik@redhat.com --- Copr build: https://copr.fedorainfracloud.org/coprs/build/5388000 (succeeded)
Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-rev...
Please take a look if any issues were found.
--- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|nobody@fedoraproject.org |benson_muite@emailplus.org Flags| |fedora-review? CC| |benson_muite@emailplus.org
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #2 from Benson Muite benson_muite@emailplus.org --- Package Review ==============
Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed
===== MUST items =====
C/C++: [x]: Package does not contain kernel modules. [x]: Package contains no static executables. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [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: [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: "GNU Library General Public License, Version 2.0", "Unknown or generated", "FSF All Permissive License", "GNU Library General Public License v2 or later", "GNU General Public License, Version 2". 8 files have unknown license. Detailed output of licensecheck in /home/FedoraPackaging/reviews/libigloo/2166288-libigloo/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: %build honors applicable compiler flags or justifies otherwise. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [s]: 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 [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. [ ]: 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. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 10240 bytes in 2 files. [ ]: 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 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: [?]: 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 libigloo-tests [ ]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [ ]: Package should compile and build into binary rpms on all supported architectures. [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]: 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: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Package should not use obsolete m4 macros [x]: Spec file according to URL is the same as in SRPM.
Rpmlint ------- Checking: libigloo-0.9.2-1.fc38.x86_64.rpm libigloo-devel-0.9.2-1.fc38.x86_64.rpm libigloo-tests-0.9.2-1.fc38.noarch.rpm libigloo-debuginfo-0.9.2-1.fc38.x86_64.rpm libigloo-debugsource-0.9.2-1.fc38.x86_64.rpm libigloo-0.9.2-1.fc38.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpzswh3bzv')] checks: 31, packages: 6
libigloo.x86_64: E: incorrect-fsf-address /usr/share/licenses/libigloo/COPYING libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/cs.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/digest.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/error.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/feature.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/init_igloo_test.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/prng.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/ro.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/tap_suite.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/time.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/uuid.c libigloo-tests.noarch: E: devel-dependency libigloo-devel libigloo.spec:181: W: configure-without-libdir-spec 6 packages and 0 specfiles checked; 2 errors, 11 warnings, 2 badness; has taken 4.3 s
Rpmlint (debuginfo) ------------------- Checking: libigloo-debuginfo-0.9.2-1.fc38.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpguxztz85')] checks: 31, packages: 1
1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 1.5 s
Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.11/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 31, packages: 5
libigloo.x86_64: E: incorrect-fsf-address /usr/share/licenses/libigloo/COPYING libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/cs.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/digest.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/error.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/feature.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/init_igloo_test.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/prng.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/ro.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/tap_suite.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/time.c libigloo-tests.noarch: W: devel-file-in-non-devel-package /usr/libexec/libigloo/tests/uuid.c libigloo-tests.noarch: E: devel-dependency libigloo-devel 5 packages and 0 specfiles checked; 2 errors, 10 warnings, 2 badness; has taken 3.1 s
Source checksums ---------------- https://downloads.xiph.org/releases/igloo/libigloo-0.9.2.tar.gz : CHECKSUM(SHA256) this package : 21896c2e4cb72a463250f8a7c1287d53a4b5882f438d296ca062a851a06942f8 CHECKSUM(SHA256) upstream package : 21896c2e4cb72a463250f8a7c1287d53a4b5882f438d296ca062a851a06942f8
Requires -------- libigloo (rpmlib, GLIBC filtered): libc.so.6()(64bit) libm.so.6()(64bit) librhash.so.0()(64bit) rtld(GNU_HASH)
libigloo-devel (rpmlib, GLIBC filtered): /usr/bin/pkg-config libigloo(x86-64) libigloo.so.0()(64bit)
libigloo-tests (rpmlib, GLIBC filtered): /usr/bin/bash autoconf automake coreutils gawk gcc libigloo-devel make pkgconf-m4
libigloo-debuginfo (rpmlib, GLIBC filtered):
libigloo-debugsource (rpmlib, GLIBC filtered):
Provides -------- libigloo: libigloo libigloo(x86-64) libigloo.so.0()(64bit)
libigloo-devel: libigloo-devel libigloo-devel(x86-64) pkgconfig(igloo)
libigloo-tests: libigloo-tests
libigloo-debuginfo: debuginfo(build-id) libigloo-debuginfo libigloo-debuginfo(x86-64) libigloo.so.0.0.0-0.9.2-1.fc38.x86_64.debug()(64bit)
libigloo-debugsource: libigloo-debugsource libigloo-debugsource(x86-64)
Generated by fedora-review 0.9.0 (6761b6c) last change: 2022-08-23 Command line :/usr/bin/fedora-review -b 2166288 Buildroot used: fedora-rawhide-x86_64 Active plugins: C/C++, Generic, Shell-api Disabled plugins: Ruby, Ocaml, fonts, PHP, Perl, Python, SugarActivity, Java, R, Haskell Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Comments: a) This mostly seems ok. b) Can a comment be added in the spec file on why the source for the tests need to be packaged? c) Should upstream add a copy of GPL license as well? d) Thanks for detailed license breakdown. The license field for the main package only contains LGPL2. Should other licenses be added?
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #3 from Petr Pisar ppisar@redhat.com ---
b) Can a comment be added in the spec file on why the source for the tests need to be packaged?
Tests are packaged in order to be able to run them later against the installed library. They will be exhibited in gating tests. I don't think this needs to be specifically documented in the spec file. There is already %description which explains what the package contains and how to use it.
c) Should upstream add a copy of GPL license as well?
Upstream think it's a copy-and-paste mistake. Next release will probably change the license to LGPL. https://gitlab.xiph.org/xiph/icecast-libigloo/-/issues/6
d) Thanks for detailed license breakdown. The license field for the main package only contains LGPL2. Should other licenses be added?
License tag always describes a content of the binary RPM package. The main binary package does not include non-LGPL files. Hence it cannot list other licenses. Compare to tests subpackage which contains tests/prng.c file, hence it lists GPL-2.0-only.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #4 from Benson Muite benson_muite@emailplus.org --- Thanks for your answers.
b) Fedora review generates many warnings and an error because of the test package. The description indicates they are utilities used in Icecast. The reason for packaging the tests is unclear to me. For most users, it seems reasonable that the tests are run during the build process, but need not be packaged for the users to run. None of the distributions listed at https://repology.org/project/libigloo/packages packages the tests.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #5 from Petr Pisar ppisar@redhat.com --- Packaging tests is specific for Fedora. The reason is to test the software after installation. There is 534 *-tests RPM packages like that in Fedora. It has no relation to the upstream.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #6 from Benson Muite benson_muite@emailplus.org --- Do the tests need the source files as well or just the test binary? For my own knowledge, is packaging of configure.ac and Makefile.am something that can be done for other packages?
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #7 from Petr Pisar ppisar@redhat.com --- The tests exhibit the binary library and header files. Both are package in libigloo-devel. The tests are packaged in source form on purpose, to exhibit libigloo API. Minified configure.ac and Makefile.am are packaged into libigloo-tests to compile and harness the tests at run-time.
Separating tests for autoconf-driven build systems is not generally easy. Hence it is rare. Contrary, libigloo is simple, so I did it. Other language have tests and build system better delineated and packaging tests is much easier. See some Perl packages where it's mostly about copying test files.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #8 from Benson Muite benson_muite@emailplus.org --- Ok. Perl is an interpreted language, so packaging source files is reasonable.
Arch does distribute source files, which can be helpful for getting better performance suited to the hardware being used if they are compiled locally. Fedora typically has not done this, though it maybe a good thing to add.
The C and C++ guidelines do not prohibit distributing source: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/ but Fortran guidelines do: https://docs.fedoraproject.org/en-US/packaging-guidelines/Fortran/#_packagin... so would expect this to be the case for all programs in compiled languages.
Checking builds on COPR for Aarch64 and S390x
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #9 from Petr Pisar ppisar@redhat.com --- I don't understand what you are trying to prove. The library is packaged as it should be. The header files, which are technically sources, are packaged as they should be, and the tests are packaged in way to test the library and header files. Having packaged tests is maybe unusual, but IMHO nothing bad.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Benson Muite benson_muite@emailplus.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |POST Flags|fedora-review? |fedora-review+
--- Comment #10 from Benson Muite benson_muite@emailplus.org --- Ok. Not proving anything, would like to do a thorough review. Packaging guidelines do not seem to cover all cases. It is quite standard to package header files. Packaging C source files is unusual, given that this is prohibited in Fortran and not stated for other compiled languages, assumed it is generally not done/prohibited.
Builds on copr completed: https://copr.fedorainfracloud.org/coprs/fed500/libigloo/build/5493006/
Approved.
Thorough review of https://bugzilla.redhat.com/show_bug.cgi?id=2165536 would be appreciated.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
--- Comment #11 from Fedora Admin user for bugzilla script actions fedora-admin-xmlrpc@fedoraproject.org --- The Pagure repository was created at https://src.fedoraproject.org/rpms/libigloo
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|POST |MODIFIED Fixed In Version| |libigloo-0.9.2-1.fc38
--- Comment #12 from Petr Pisar ppisar@redhat.com --- Thank you for the review and repository.
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
https://bugzilla.redhat.com/show_bug.cgi?id=2166288
Petr Pisar ppisar@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ON_QA |CLOSED Resolution|--- |RAWHIDE Last Closed| |2023-02-07 13:44:17
package-review@lists.fedoraproject.org
-
bugzilla＠redhat.com