https://bugzilla.redhat.com/show_bug.cgi?id=2468312
Cristian Le <[email protected]> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |[email protected] Blocks| |177841 (FE-NEEDSPONSOR) Status|NEW |ASSIGNED Assignee|[email protected] |[email protected] --- Comment #2 from Cristian Le <[email protected]> --- So there's a bunch of things to comment here, let me go through the order they appear in the spec file: - Whenever possible, please use `%autorelease`/`%autochangelog` [1], it helps other contributors, particularly with rebasing PRs - The `Source0` should be an url, in this case `https://github.com/pwsafe/pwsafe/archive/refs/tags/%{version}.tar.gz` or equivalent - License must be in SPDX format: `Artistic-2.0` - 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 - We do not use Group/Packager fields, and especially not Vendor - Please narrow down the `BuildRequires`, `make` for example is obviously not used here - `Requires` and handled automagically, particularly for libraries. You MUST NOT add them manually as these can change out of your control - For the `Obsoletes` see the packaging guideline on this [2] - It is best to use `%autosetup` or `%autopatch` to make adding/removing patches easier - Unless you need to port this to an old RHEL, include a `%conf` section which would have the `%cmake` (configure) step - 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. - 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? - I do not see value for converting the markdown and other such text files into pdf - 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 - `%clean` is deprecated - 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.) - 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 - The %defattr is unnecessary afaict [1]: https://fedora-infra.github.io/rpmautospec-docs/index.html [2]: https://docs.fedoraproject.org/en-US/packaging-guidelines/#renaming-or-replacing-existing-packages [3]: https://docs.fedoraproject.org/en-US/packaging-guidelines/CMake/ Referenced Bugs: https://bugzilla.redhat.com/show_bug.cgi?id=177841 [Bug 177841] Tracker: Review requests from new Fedora packagers who need a sponsor -- 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%23c2 -- _______________________________________________ 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
