https://bugzilla.redhat.com/show_bug.cgi?id=2468312
--- Comment #3 from Carl Byington <[email protected]> --- - Whenever possible, please use `%autorelease`/`%autochangelog` [1], it helps other contributors, particularly with rebasing PRs I don't think the upstream git repo is compatible with that. - The `Source0` should be an url, in this case `https://github.com/pwsafe/pwsafe/archive/refs/tags/%{version}.tar.gz` or equivalent done. - License must be in SPDX format: `Artistic-2.0` done. - We would need to do a proper license check for this one since often there are undocumented embedded files with different license/copyright. At the very least the Yubico license files stand out I have removed the Yubico license files from the package. There is no code in this project from Yubico - although it links to both libyubikey and ykpers. The license files here are copies of the license files in libyubikey and ykpers. But we surely don't need to include license files from every package that we link to? - We do not use Group/Packager fields, and especially not Vendor done. - Please narrow down the `BuildRequires`, `make` for example is obviously not used here I need access to the packager group to run scratch builds to see which of those are already included in the standard build environment and can be removed. - `Requires` and handled automagically, particularly for libraries. You MUST NOT add them manually as these can change out of your control done. - For the `Obsoletes` see the packaging guideline on this [2] There is an existing Fedora pwsafe package that is an ancient cli only program. It provides /usr/bin/pwsafe which conflicts with this package. The maintainer is going to retire that package. This package Obsoletes every version of that old package. - It is best to use `%autosetup` or `%autopatch` to make adding/removing patches easier done. - Unless you need to port this to an old RHEL, include a `%conf` section which would have the `%cmake` (configure) step done. - In the `%build`, please follow the cmake guidelines [3], specifically, use the `%cmake*` macros, do not hard-code the generator, do not manually handle the builddir, etc. done. - A review of the available build flags is needed, the linkage, vendoring of source, etc. Did you notice anything that I should keep in mind before diving into it? No. - I do not see value for converting the markdown and other such text files into pdf Consider an end-user who wants to read the doc files. Hardly any end-users are familiar with either rtf or markdown. On my F44 system, file manager right click on .rtf suggests wordpad, which does not exist in Fedora. Right click on .md suggests emacs which won't display it nicely. End-users are familiar with .pdf, I think evince is installed by default, and file manager right click opens it. - What's the reason for deleting some of the source files in the %build phase? It would make debugging in `mock` more difficult at the very least done. Upstream docs directory is cluttered with files, so we only package the ones that are relevant. - `%clean` is deprecated done. - Please do not use manual `install` command. Everything should be handled in either `%cmake_install` (if not available, please raise an issue to upstream), or `%files` (for `LICENSE`, `%doc`, etc.) %cmake_install does almost everything, but it does not install any of the docs files. I install those manually via `install`. - There is something missing that should be done with the icons, but I need to cross-reference some other packages - Same for the translation files - `%check` section is missing, and I can tell that they have ctest tests If we use `%cmake` rather than `%cmake -DNO_GTEST=ON`, it does a git clone to download googletest source code, which is not allowed in Fedora. But in that case it does find two tests which pass. - The %defattr is unnecessary afaict done. Updated spec and srpm Spec URL: https://www.five-ten-sg.com/util/passwordsafe.spec SRPM URL: https://www.five-ten-sg.com/util/passwordsafe-1.24.0-1.fc44.src.rpm -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2468312 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202468312%23c3 -- _______________________________________________ package-review mailing list -- [email protected] To unsubscribe send an email to [email protected] 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/[email protected] Do not reply to spam, report it: https://forge.fedoraproject.org/infra/tickets/issues/new
