https://bugzilla.redhat.com/show_bug.cgi?id=2115102 --- Comment #22 from Wayne Sun <gsun@xxxxxxxxxx> --- (In reply to Miro Hrončok from comment #11) > 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. For the downstream I would like the spec host on Fedora git which for my understanding is: https://src.fedoraproject.org/ While for request a repo on fedora git with: $fedpkg request-repo pylero 2115102 Could not execute request_repo: The Bugzilla bug is not approved yet So it only could be proceed when the bug got approved. I've updated the upstream spec: https://github.com/RedHatQE/pylero/blob/main/pylero.spec which I'll reply inline to confirm what have changed. > > > =============== > > > 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. > Updated, the 16th column does look better. > > 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. > Updated > > 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 > The python3-setuptools is removed as it's not required. > > 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. > > Updated per recommendation. > 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. > Updated with not change the setuptools_scm in spec > > 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. > > So the pylero CLI is not recommended for fedora as it prefer gnureadline package (non-fedora release) over native python3 readline package, this should not be an issue for packaging, I have removed the 'rm' part. > 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. > Updated with using pyproject_files > > 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.""" The changelog haven't been updated current spec file is upstream and downstream in fedora git will remove it. For issue 9: I have updated the package name to python-pylero as it's preferred to be used as library. For issue 10: Yeah, I have added the %pyproject_check_import under %check Thanks for reviewing and explain all the details. -- 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
