https://bugzilla.redhat.com/show_bug.cgi?id=2115102 --- Comment #11 from Miro Hrončok <mhroncok@xxxxxxxxxx> --- First of all, I am looking at https://raw.githubusercontent.com/RedHatQE/pylero/main/pylero.spec -- I understand from the RUL that this spec file lives in the upstream repository at https://github.com/RedHatQE/pylero/blob/main/pylero.spec and that you've been updating it there when receiving feedback from Adam. This looks like a classic example of downstream == upstream, which makes some things easier but it can also create some friction. I recommend reading this first: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_spec_maintenance_and_canonicity If you are OK with that, it's fine. =============== Next, I read the spec file line by line and will note down anything that I do not understand or consider bad practice. Note that there are the packaging guidelines and I also have opinions :) I will try to make it clear if I don't like something personally (and I will explain why) or if something is specified in the guidelines. It's completely fine to disagree witch my opinions, but I would appreciate if you could explain why when you choose to do that. 1) [OPINION] The values (right side of : after Name, Version, etc.) are not aligned to the 16th column. That makes the specfile different from the one listed at https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_example_spec_file and in my opinion, harder to read. There is however no guideline to align to the 16th column. I just consider it the de-facto standard at least for Python packages. 2) [OPINION] Unlike the example Python specfile, this specfile uses "Source0:" instead of "Source:". Looking at the conversation here, this was changed from "SOURCE:" to "Source0:" based on Adam's comment #1. While I agree that "SOURCE:" is not typical at all, I disagree that the number 0 should be there. Many packages use it as it was the standard until quite recently, but the packaging guidelines were updated to not use such numbers were not necessary in https://pagure.io/packaging-committee/pull-request/1157 -- I highly recommend watching this talk from DevConf.CZ 2020 titled "Still packaging like it's 1999?": https://youtu.be/0bfA0441dxc?t=666 -- it explains the numbers and other things. There is however no guideline to not using the redundant number. I just consider redundant things from the previous decade a tech debt. 3) [MUST NOT] There is "BuildRequires: python3-setuptools" in the spec file, but the spec file uses %pyproject_buildrequires and the dependency is generated from https://github.com/RedHatQE/pylero/blob/0.0.4/pyproject.toml#L2 -- hence the manual BuildRequires tag is redundant and the guidelines explicitly say: "Automatically determined dependencies MUST NOT be duplicated by manual dependencies." in https://docs.fedoraproject.org/en-US/packaging-guidelines/#_package_dependencies 4) [OPINION] The %description uses Markdown. The descriptions of RPM packages are not explicitly formatted with any markup. Packagers get creative sometimes but the content will always be rendered as plaintext. I consider using Markdown (other than *s for emphasis and similar "easy" constructs) confusing. Especially when the description starts with a #-style heading. I am also not entirely sure "Welcome to Pylero" is a typical description. I'd start with "This is Pylero". There is however no guideline to not using Markdown or to greet users in descriptions. I just consider both highly rare. 5) [CONFUSION] The setuptools_scm dependency: I understand the comment, but I don't understand the rationale. *Why* is it removed? Is it just because we can live without it and we want to make the dependency footprint as small as possible? Or does the package build wrongly when setuptools_scm is installed? If this is the footprint case, I'd drop it entirely, as it makes the specfile a tiny bit more complex than necessary for a very little benefit. If this is the second case, it needs to be explicitly mentioned in the comment. It might also require a BuildConflicts tag in that case. 6) [CONFUSION] Why "rm -f %{buildroot}%{_bindir}/pylero"? What is the rationale for not shipping the file? Adam says that if you are sure you don't want the file, you should rm it instead of using %_unpackaged_files_terminate_build. I completely agree with that, but I don't understand why do you want to get rid of it. Once we know, the reason should IMHO be documented ina comment. It is also my opinion that spec file should not use rm -f. If the file is no longer there, the rm should be removed from the specfile -- but with rm -f you will never notice. 7) [OPINION] The %files section does not use %{pyproject_files} (and the %install section does not use %pyproject_save_files). I consider using this the best practice, but there is no guideline that would say this is a MUST or SHOULD. 8) [SHOULD NOT] The %changelog is quite verbose for downstream. Usually, downstream changelog entries say "Updated to 0.0.4" or similar. No need to reuse the upstream changelog here. See https://docs.fedoraproject.org/en-US/packaging-guidelines/#changelogs """Changelog entries should provide a brief summary of the changes done to the package between releases, including noting updating to a new version, adding a patch, fixing other spec sections, note bugs fixed, and CVE’s if any. They must never simply contain an entire copy of the source CHANGELOG entries. The intent is to give the user a hint as to what changed in a package update without overwhelming them with the technical details. Links to upstream changelogs can be entered for those who want additional information.""" -- 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=2115102 _______________________________________________ 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
