https://bugzilla.redhat.com/show_bug.cgi?id=2318522 --- Comment #1 from wojnilowicz <lukasz.wojnilowicz@xxxxxxxxx> --- Hi, this is my first review of a c++ package, so please bear with me :) Here is the list of things that I see as issues: 1) Shouldn't the "Path:" statements be numbered as at https://docs.fedoraproject.org/en-US/packaging-guidelines/#_applying_patches ? 2) Could you also add a link to the PR and not only a link to the patches? It's difficult to locate a PR based only on https://github.com/deskflow/deskflow/pull/7651.patch 3) Are below really required? I don't see them in the CMake files? BuildRequires: gulrak-filesystem-devel BuildRequires: libcurl-devel BuildRequires: cmake(Qt6Core5Compat) BuildRequires: cmake(Qt6LinguistTools) BuildRequires: pkgconfig(avahi-compat-libdns_sd) 4) On the master branch it is 3.24 now. What do you think about bumping it to that or dropping it altogther (as required at https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependencies or maybe at https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/#_build_dependencies)? BuildRequires: cmake >= 3.12 5) Shouldn't you drop versioned dependencies here as required at https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependencies ? BuildRequires: pkgconfig(libei-1.0) >= 0.99.1 BuildRequires: pkgconfig(libportal) >= 0.8.0 6) CMake will fail if HAVE_PTHREAD is FALSE. Shouldn't you add then the following line? BuildRequires: glibc-devel 7) Shouldn't you add https://github.com/deskflow/deskflow/blob/v1.17.0/LICENSE_EXCEPTION to %license as well? 8) I haven't built it yet, but shouldn't more go under the %doc tag? I mean products of mainpage.md and configuration.md. They're built as doc in CMake. Other than that it LGTM. I would like it to build though, and use a proper review template, so I'm hopping that adding the following lines: Spec URL: https://ngompa.fedorapeople.org/for-review/deskflow.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/deskflow-1.17.0-1.fc40.src.rpm will trigger the build. -- 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=2318522 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202318522%23c1 -- _______________________________________________ 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
