Bug 2154918 - Review Request: perl-Text-QRCode - Perl module to generate text base QR Code
Summary: Review Request: perl-Text-QRCode - Perl module to generate text base QR Code
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Petr Pisar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2022-12-19 15:51 UTC by Johan Vromans
Modified: 2023-02-17 18:16 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2023-02-17 18:16:14 UTC
Type: ---
Embargoed:
ppisar: fedora-review+


Attachments (Terms of Use)

Description Johan Vromans 2022-12-19 15:51:48 UTC
Spec URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode.spec
SRPM URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode-0.05-1.fc37.src.rpm

Description:
Perl module Text::QRCode provides bindings for the libqrencode library.

Fedora Account System Username: sciurius

Comment 1 Petr Pisar 2023-02-03 15:19:46 UTC
URL and Source0 addresses are Ok.
Source0 archive (SHA-512: 2941d15da53dbbe7be6ebf691d08e78146835acc7891dd2d45bbdf6678a6d824044e359d34fde8c7fe1c2ba0eb10feaae4f1162ecc71f821cd2ead19f9bd4085) is original. Ok.

FIX: Remove a "Text::QRCode -" prefix from the summary. We do not repeat the module name in RPM summary. Compare to other Perl packages. Also a summary of this review bug must match "%{name} - %{summary}" value from the spec file.

TODO: Remove "This module requies libqrencode '2.0.0' and above." sentence from the description. This information is not helpful to users. The dependency is already stated and assured with RPM dependencies.

License verified from README, ppport.h, lib/Text/QRCode.pm, inc/Module/Install.pm. Ok.
FIX: Convert a License tag to SPDX format. Fedora moved a license syntax to SPDX <https://docs.fedoraproject.org/en-US/legal/license-field/#_perl_packages>.

FIX: Remove "Requires: perl(:MODULE_COMPAT_..." line from the spec file. The dependency is now automatically generated with perl-generators.

FIX: Either unbundle modules found in ./inc ("rm -r ./inc/*" in %prep) and build-require the few inc::Module::Install modules (Makefile.PL:3), or list all dependencies of ./inc code (e.g. perl(Config) at inc/Module/Install/Can.pm:5). I recommend you the first option as it is much easier and Fedora-way. You can use 'tangerine' tool for enumerating Perl dependencies.

FIX: Build-require 'gcc' and 'perl-devel' for building XS modules.

FIX: Build-require 'perl(File::Copy)' (Makefile.PL:4).
FIX: Build-require 'pkgconf-pkg-config' (Makefile.PL:31).
FIX: Build-require 'pkgconfig(libqrencode)' (Makefile.PL:44).
FIX: Build-require 'perl(base)' (lib/Text/QRCode.pm:5).
FIX: Build-require 'perl(vars)' (lib/Text/QRCode.pm:6).
FIX: Build-require 'perl(XSLoader)' (lib/Text/QRCode.pm:15).

Do not build-require 'perl(parent)', 'perl(utf8)'. I can see where they are used.

FIX: Explicitly list packaged manual pages and %{perl_vendorarch}/auto files <https://docs.fedoraproject.org/en-US/packaging-guidelines/#_explicit_lists>.

All tests pass. Ok.

$ rpmlint perl-Text-QRCode.spec ../SRPMS/perl-Text-QRCode-0.05-1.fc38.src.rpm ../RPMS/x86_64/perl-Text-QRCode-*
======================================== 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

========= 4 packages and 1 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.3 s ========
rpmlint is Ok.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Text-QRCode-0.05-1.fc38.x86_64.rpm 
drwxr-xr-x    2 root     root                        0 Dec 19 01:00 /usr/lib/.build-id
drwxr-xr-x    2 root     root                        0 Dec 19 01:00 /usr/lib/.build-id/bf
lrwxrwxrwx    1 root     root                       66 Dec 19 01:00 /usr/lib/.build-id/bf/295d08b666a3d82947c1fdb4b9459e9ff93cb0 -> ../../../../usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
-rw-r--r--    1 root     root                     2850 Oct 12  2016 /usr/lib64/perl5/vendor_perl/Text/QRCode.pm
drwxr-xr-x    2 root     root                        0 Dec 19 01:00 /usr/lib64/perl5/vendor_perl/auto/Text
drwxr-xr-x    2 root     root                        0 Dec 19 01:00 /usr/lib64/perl5/vendor_perl/auto/Text/QRCode
-rwxr-xr-x    1 root     root                    15872 Dec 19 01:00 /usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
drwxr-xr-x    2 root     root                        0 Dec 19 01:00 /usr/share/doc/perl-Text-QRCode
-rw-r--r--    1 root     root                      828 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/Changes
-rw-r--r--    1 root     root                     1396 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/README
-rw-r--r--    1 root     root                     1690 Dec 19 01:00 /usr/share/man/man3/Text::QRCode.3pm.gz
FIX: Package also /usr/lib64/perl5/vendor_perl/Text directory.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Text-QRCode-0.05-1.fc38.x86_64.rpm | sort -f | uniq -c
      1 ld-linux-x86-64.so.2()(64bit)
      1 ld-linux-x86-64.so.2(GLIBC_2.3)(64bit)
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libc.so.6(GLIBC_2.4)(64bit)
      1 libperl.so.5.36()(64bit)
      1 libqrencode.so.4()(64bit)
      2 perl(:MODULE_COMPAT_5.36.0)
      1 perl(base)
      1 perl(Carp)
      1 perl(Exporter)
      1 perl(strict)
      1 perl(vars)
      1 perl(warnings)
      1 perl-libs
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsZstd) <= 5.4.18-1
      1 rtld(GNU_HASH)
FIX: Do not require perl(:MODULE_COMPAT_...) explicitly.
FIX: Require 'perl(XSLoader)' (lib/Text/QRCode.pm:15).

$ rpm -q --provides -p ../RPMS/x86_64/perl-Text-QRCode-0.05-1.fc38.x86_64.rpm | sort -f | uniq -c
      1 perl(Text::QRCode) = 0.05
      1 perl-Text-QRCode = 0.05-1.fc38
      1 perl-Text-QRCode(x86-64) = 0.05-1.fc38
Binary provides are Ok.

$ resolvedeps rawhide ../RPMS/x86_64/perl-Text-QRCode-0.05-1.fc38.x86_64.rpm 
Binary dependencies are resolvable. Ok.

The package builds in F38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=97051151). Ok.

Please correct all 'FIX' items, consider fixing the 'TODO' item, and provide a new spec file.

Comment 2 Johan Vromans 2023-02-03 21:08:19 UTC
Thanks for the feedback. New spec and srpm at:

Spec URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode.spec
SRPM URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode-0.05-2.fc37.src.rpm

(In reply to Petr Pisar from comment #1)

> FIX: Remove a "Text::QRCode -" prefix from the summary.

Done.

> TODO: Remove "This module requies libqrencode '2.0.0' and above." sentence
> from the description.

Done.

> FIX: Convert a License tag to SPDX format. Fedora moved a license syntax to
> SPDX

Done.

> FIX: Remove "Requires: perl(:MODULE_COMPAT_..." line from the spec file.

Done.

> FIX: Either unbundle modules found in ./inc ("rm -r ./inc/*" in %prep) and
> build-require the few inc::Module::Install modules

Done.

> FIX: Build-require 'gcc' and 'perl-devel' for building XS modules.
> FIX: Build-require 'perl(File::Copy)' (Makefile.PL:4).
> FIX: Build-require 'pkgconf-pkg-config' (Makefile.PL:31).
> FIX: Build-require 'pkgconfig(libqrencode)' (Makefile.PL:44).
> FIX: Build-require 'perl(base)' (lib/Text/QRCode.pm:5).
> FIX: Build-require 'perl(vars)' (lib/Text/QRCode.pm:6).
> FIX: Build-require 'perl(XSLoader)' (lib/Text/QRCode.pm:15).

Done.

> Do not build-require 'perl(parent)', 'perl(utf8)'. 

Done.

> FIX: Explicitly list packaged manual pages and %{perl_vendorarch}/auto files

Done.

> FIX: Package also /usr/lib64/perl5/vendor_perl/Text directory.

I'm not sure I understand. If I add %{perl_vendorarch}/Text to %files I get a rpm build warning that %{perl_vendorarch}/Text/QRCode.pm is listed twice. Using %dir %{perl_vendorarch}/Text is not correct since this module does not own this directory.

> FIX: Do not require perl(:MODULE_COMPAT_...) explicitly.
> FIX: Require 'perl(XSLoader)' (lib/Text/QRCode.pm:15).

Already done.

Thanks!

Comment 4 Petr Pisar 2023-02-06 08:46:35 UTC
> > FIX: Either unbundle modules found in ./inc ("rm -r ./inc/*" in %prep) and build-require the few inc::Module::Install modules (Makefile.PL:3), or list all dependencies of ./inc code (e.g. perl(Config) at inc/Module/Install/Can.pm:5). I recommend you the first option as it is much easier and Fedora-way.

+BuildRequires: perl(Module::Install)

FIX: Build-require 'perl(inc::Module::Install)' instead of 'perl(Module::Install)'. That's the module which is loaded (Makefile.PL:3).

+BuildRequires: perl-Module-Install

TODO: Although a dependency on an RPM package also works, in Perl we prefer depending in modules because modules sometimes move between packages. I know that perl-Module-Install is weird with its automatically loaded plugins. Here is a list of the modules whose subroutines are called by Makefile.PL:

perl(Module::Install::Metadata)
perl(Module::Install::Compiler)
perl(Module::Install::Can)
perl(Module::Install::AutoInstall)
perl(Module::Install::WriteAll)

Try depending on them instead of perl-Module-Install.


FIX: Build-require 'coreutils' (perl-Text-QRCode.spec:40).

$ rpmlint perl-Text-QRCode.spec ../SRPMS/perl-Text-QRCode-0.05-2.fc38.src.rpm ../RPMS/x86_64/perl-Text-QRCode-*
======================================== 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

========= 4 packages and 1 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.3 s ========
rpmlint is Ok.


> > FIX: Package also /usr/lib64/perl5/vendor_perl/Text directory.
> I'm not sure I understand. If I add %{perl_vendorarch}/Text to %files I get a rpm build warning that %{perl_vendorarch}/Text/QRCode.pm is listed twice. Using %dir %{perl_vendorarch}/Text is not correct since this module does not own this directory.

Using both "%dir %{perl_vendorarch}/Text" and "%{perl_vendorarch}/Text/QRCode.pm" is correct. The package will co-own the directory with other packages. We do it so because the package cannot guarantee that there will always be another package installed which would own that directory. Hence what we do is that we own the full path except the top-level %{perl_vendorarch} which is owned by perl-libs. Strictly reading guidelins, recursively owning "%{perl_vendorarch}/Text" would be enough. But since the package is named Text-QRCode, an intention is to occupy Text::QRCode name space, and therefore we tend to explicitly own all path components under %{perl_vendorarch} and up to %{perl_vendorarch}/Text/QRCode*.

$ rpm -q -lv -p ../RPMS/x86_64/perl-Text-QRCode-0.05-2.fc38.x86_64.rpm 
drwxr-xr-x    2 root     root                        0 Feb  3 01:00 /usr/lib/.build-id
drwxr-xr-x    2 root     root                        0 Feb  3 01:00 /usr/lib/.build-id/4c
lrwxrwxrwx    1 root     root                       66 Feb  3 01:00 /usr/lib/.build-id/4c/fb2219d45a654fa167615a162bf353250f0cd5 -> ../../../../usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
-rw-r--r--    1 root     root                     2850 Oct 12  2016 /usr/lib64/perl5/vendor_perl/Text/QRCode.pm
-rwxr-xr-x    1 root     root                    15872 Feb  3 01:00 /usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
drwxr-xr-x    2 root     root                        0 Feb  3 01:00 /usr/share/doc/perl-Text-QRCode
-rw-r--r--    1 root     root                      828 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/Changes
-rw-r--r--    1 root     root                     1396 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/README
-rw-r--r--    1 root     root                     1690 Feb  3 01:00 /usr/share/man/man3/Text::QRCode.3pm.gz

FIX: Own %{perl_vendorarch}/Text and %{perl_vendorarch}/auto/Text directories.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Text-QRCode-0.05-2.fc38.x86_64.rpm | sort -f | uniq -c 
      1 ld-linux-x86-64.so.2()(64bit)
      1 ld-linux-x86-64.so.2(GLIBC_2.3)(64bit)
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libc.so.6(GLIBC_2.4)(64bit)
      1 libperl.so.5.36()(64bit)
      1 libqrencode.so.4()(64bit)
      1 perl(:MODULE_COMPAT_5.36.0)
      1 perl(base)
      1 perl(Carp)
      1 perl(Exporter)
      1 perl(strict)
      1 perl(vars)
      1 perl(warnings)
      1 perl-libs
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsZstd) <= 5.4.18-1
      1 rtld(GNU_HASH)

FIX: Explicitly require 'perl(XSLoader)' (lib/Text/QRCode.pm:15). That dependencies is not automatically generated because it is loaded at run-time.

The package builds in F38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=97181255). Ok.

You did great. The package is almost perfect. Please correct the FIX items, consider fixing a TODO item, and provide a new spec file.

Comment 5 Johan Vromans 2023-02-07 18:23:38 UTC
Thanks for the feedback. New spec and srpm at:

Spec URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode.spec
SRPM URL: https://www.squirrel.nl/pub/xfer/perl-Text-QRCode-0.05-3.fc37.src.rpm

All FIX and TODO items are addressed. Thanks for the detailed explanations.

Comment 6 Petr Pisar 2023-02-08 08:01:33 UTC
$ rpm -q -lv -p ../RPMS/x86_64/perl-Text-QRCode-0.05-3.fc38.x86_64.rpm 
drwxr-xr-x    2 root     root                        0 Feb  7 01:00 /usr/lib/.build-id
drwxr-xr-x    2 root     root                        0 Feb  7 01:00 /usr/lib/.build-id/3b
lrwxrwxrwx    1 root     root                       66 Feb  7 01:00 /usr/lib/.build-id/3b/9f962850b3b32bac4a554abfe84ee46aff7f55 -> ../../../../usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
drwxr-xr-x    2 root     root                        0 Feb  7 01:00 /usr/lib64/perl5/vendor_perl/Text
-rw-r--r--    1 root     root                     2850 Oct 12  2016 /usr/lib64/perl5/vendor_perl/Text/QRCode.pm
drwxr-xr-x    2 root     root                        0 Feb  7 01:00 /usr/lib64/perl5/vendor_perl/auto/Text/QRCode
-rwxr-xr-x    1 root     root                    15872 Feb  7 01:00 /usr/lib64/perl5/vendor_perl/auto/Text/QRCode/QRCode.so
drwxr-xr-x    2 root     root                        0 Feb  7 01:00 /usr/share/doc/perl-Text-QRCode
-rw-r--r--    1 root     root                      828 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/Changes
-rw-r--r--    1 root     root                     1396 Oct 12  2016 /usr/share/doc/perl-Text-QRCode/README
-rw-r--r--    1 root     root                     1690 Feb  7 01:00 /usr/share/man/man3/Text::QRCode.3pm.gz
FIX: You also need to own %{perl_vendorarch}/auto/Text directory.

$ rpm -q --requires -p ../RPMS/x86_64/perl-Text-QRCode-0.05-3.fc38.x86_64.rpm | sort -f | uniq -c
      1 ld-linux-x86-64.so.2()(64bit)
      1 ld-linux-x86-64.so.2(GLIBC_2.3)(64bit)
      1 libc.so.6()(64bit)
      1 libc.so.6(GLIBC_2.2.5)(64bit)
      1 libc.so.6(GLIBC_2.4)(64bit)
      1 libperl.so.5.36()(64bit)
      1 libqrencode.so.4()(64bit)
      1 perl(:MODULE_COMPAT_5.36.0)
      1 perl(base)
      1 perl(Carp)
      1 perl(Exporter)
      1 perl(strict)
      1 perl(vars)
      1 perl(warnings)
      1 perl(XSLoader)
      1 perl-libs
      1 rpmlib(CompressedFileNames) <= 3.0.4-1
      1 rpmlib(FileDigests) <= 4.6.0-1
      1 rpmlib(PayloadFilesHavePrefix) <= 4.0-1
      1 rpmlib(PayloadIsZstd) <= 5.4.18-1
      1 rtld(GNU_HASH)
Binary requires are Ok.

The package builds in F38 (https://koji.fedoraproject.org/koji/taskinfo?taskID=97260218). Ok.

Please correct the FIX item before building this package.
Resolution: Package APPROVED.

Comment 7 Fedora Admin user for bugzilla script actions 2023-02-08 12:48:26 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/perl-Text-QRCode

Comment 8 Johan Vromans 2023-02-08 14:13:25 UTC
Fixed, thanks.

Comment 9 Fedora Update System 2023-02-08 14:18:22 UTC
FEDORA-2023-46a4f30f35 has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2023-46a4f30f35

Comment 10 Fedora Update System 2023-02-09 10:09:12 UTC
FEDORA-2023-46a4f30f35 has been pushed to the Fedora 37 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-46a4f30f35 \*`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-46a4f30f35

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 11 Fedora Update System 2023-02-17 18:16:14 UTC
FEDORA-2023-46a4f30f35 has been pushed to the Fedora 37 stable repository.
If problem still persists, please make note of it in this bug report.


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