https://bugzilla.redhat.com/show_bug.cgi?id=2161942 Petr Menšík <pemensik@xxxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(pemensik@redhat.c | |om) | --- Comment #7 from Petr Menšík <pemensik@xxxxxxxxxx> --- Spec URL: https://pemensik.fedorapeople.org/bind9-next.spec SRPM URL: https://pemensik.fedorapeople.org/srpm/bind9-next-9.19.8-2.fc38.src.rpm dist-git: https://src.fedoraproject.org/fork/pemensik/rpms/bind/tree/bind9-dev (In reply to Petr Špaček from comment #6) > Hi, here is my review. Please search for [?] and [!], there is couple of > them. > > Besides that the auto-generated Issues section is totally confusing me, > please have a look there as well to double check. > > Issues: > ======= > - Package is not relocatable. > Comment: Not really an issue as it is not intended to be. > > - Sources used to build the package match the upstream source, as provided > in the spec URL. > Note: Upstream MD5sum check error, diff is in /var/lib/copr- > rpmbuild/results/bind9-next/diff.txt > See: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ > > Huh? I can't see the problem. Can you spot it? Have no idea what this means. Never seen that and do not understand it either. > > > ===== MUST items ===== > > C/C++: > [x]: Package does not contain kernel modules. > [x]: Package contains no static executables. > [x]: Development (unversioned) .so files in -devel subpackage, if present. > Unversioned so-files in private %_libdir subdirectory are okay. > [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]: Package does not contain any libtool archives (.la) > [x]: Rpath absent or only used for internal libs. > > 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. > [x]: License field in the package spec file matches the actual license. > [x]: License file installed when any subpackage combination is installed. > [?]: Package requires other packages for directories it uses. > Note: No known owner of /var/named/chroot/usr/share > Seems like this one needs attention? I'm confused because this directory is > not present in the built RPM. > > [?]: Package must own all directories that it creates. > Note: Directories without known owners: /etc/logrotate.d, > /var/named/chroot/usr/share > Ditto. I'm confused because chroot/usr/share directory is not present in the > built RPM. That is the error. If installed, it creates that directory. When uninstalled, unowned directory is left there and not properly deleted. Because it is not official part of the package. > ... > ===== SHOULD items ===== > > Generic: > [!]: Reviewer should test that the package builds in mock. > [!]: Uses parallel make %{?_smp_mflags} macro. %make_build macro contains those flags, see rpm -E %make_build. But pushed make_build to remaining places also, -j may override them when needed. > [-]: If the source package does not include license text(s) as a separate > file from upstream, the packager SHOULD query upstream to include it. > [!]: Final provides and requires are sane (see attachments). > Please see rpmlint output in COPR. It complains about > E: missing-dependency-to-logrotate for logrotate I want just to include logrotate file in bind, but do not depend or suggest logrotate. Default configuration logs to systemd journal and that does not need any logrotate. Owned the logrotate directory, but that is all I want. > > Besides that, can you double check the rest, most notably W: > obsolete-not-provided ? That one looks okay to me, but please double check. > > [?]: Fully versioned dependency in subpackages if applicable. > Note: No Requires: %{name}%{?_isa} = %{version}-%{release} in > bind9-next-libs , bind9-next-license , bind9-next-utils , bind9-next- > dnssec-utils , bind9-next-devel , bind9-next-chroot , bind9-next-dlz- > filesystem , bind9-next-dlz-ldap , bind9-next-dlz-mysql , bind9-next- > dlz-sqlite3 > I suppose this is okay because there is the -chroot variant. Can you confirm > I got it right? I think it is confused by: Requires: %{name}-license = %{epoch}:%{version}-%{release} or Requires: %{name}%{?_isa} = %{epoch}:%{version}-%{release} I think they are okay. > > [x]: Package functions as described. > [x]: Latest version is packaged. > [x]: Package does not include license text files separate from upstream. > > [!]: Patches link to upstream bugs/comments/lists or are otherwise > justified. > Some patches are missing justification, like -PIE with FIXME on it. > Especially the -next version should ideally have no patches. Pretty please! > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_patch_guidelines Okay, I admit that is my intention. This weird thingy were added in commit: https://src.fedoraproject.org/rpms/bind/c/bbeea42ab3194cda396fdf32e9ee516cec4bb3ca I guess I will have to do more research what exactly it tried to fix and whether it still make sense to do. If it does, it clearly should be offered upstream. Commenting out that patch file, but keeping it in my repo. > > [x]: Scriptlets must be sane, if used. > [x]: Sources are verified with gpgverify first in %prep if upstream > publishes signatures. > Note: Sources 1, 3, 16, 17, 18, 19, 20, 23, 25, 27, 35, 36, 37, 38, > 41, 42, 43, 44, 46, 48 and 49 are not passed to gpgverify - but that's > okay as they are distro files. That is correct, those are downstream files and are not signed by anyone. That is okay, signatures are useful on executed code. That is covered. > [x]: Package should compile and build into binary rpms on all supported > architectures. > [x]: %check is present and all tests pass. > Well, pass to an extent possible. Without network in Koji it's going to be > just pretension of %check anyway. > > [!]: Packages should try to preserve timestamps of original installed > files. > install -p should be prefered, see > https://docs.fedoraproject.org/en-US/packaging-guidelines/#_timestamps Oh, never noticed that, fixed! > > [x]: Files in /run, var/run and /var/lock uses tmpfiles.d when appropriate > > [!]: Spec use %global instead of %define unless justified. > Note: %define requiring justification: %define bind_export_libs isc > dns isccfg irs, %define upname_compat() %if "%{name}" != "%{upname}" > %if 0%{?fedora} >= 37 Provides: %1 = %{epoch}:%{version}-%{release} > %endif Obsoletes: %1 < 32:9.17.0 Conflicts: %1 %endif, %define > _configure "../configure", %define unit_prepare_build() find lib -name > 'K*.key' -exec cp -uv '{}' "%{1}/{}" ';' find lib -name 'testdata' > -type d -exec cp -Tav '{}' "%{1}/{}" ';' find lib -name 'testkeys' > -type d -exec cp -Tav '{}' "%{1}/{}" ';', %define > systemtest_prepare_build() cp -Tuav bin/tests "%{1}/bin/tests/", > %define chroot_fix_devices() if [ $1 -gt 1 ]; then for DEV in > "%{1}/dev"/{null,random,zero}; do if [ -e "$DEV" -a "$(/bin/stat > --printf="%G %a" "$DEV")" = "root 644" ]; then /bin/chmod 0664 "$DEV" > /bin/chgrp named "$DEV" fi done fi > > Guideline likes %global over %define. Can you improve it? > https://docs.fedoraproject.org/en-US/packaging-guidelines/ > #_global_preferred_over_define I think they are used as macro function, where %define is still appropriate. Removed one place where it was used just instead global value, but were unused. > > [x]: Buildroot is not present > -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component https://bugzilla.redhat.com/show_bug.cgi?id=2161942 _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
