Spec URL: https://raw.githubusercontent.com/RedHatQE/pylero/main/pylero.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/waynesun20/pylero/fedora-rawhide-x86_64/04867302-pylero/pylero-0.0.4-1.fc38.src.rpm Description: The Pylero wrapper enables native python access to Polarion objects and functionality using object oriented structure and functionality. Fedora Account System Username: waynesun20
I'm not a sponsor so cannot do the official review, but a couple of unofficial notes: * I'd be concerned about `%define _unpackaged_files_terminate_build 0`. I can't think of many cases where that's a good idea for a Fedora spec. Why is it here? What packages are showing up as 'unpackaged'? Is it because of the `%exclude %{_bindir}/%{name}` - you don't want that file packaged, for some reason? I would usually handle that by just deleting it at the end of `%install`, which avoids the need for `_unpackaged_files_terminate_build` and a `%exclude`. * Why is unmangled_version defined but never used? Does a macro use it? I don't know that any of the standard Python macros do. If it's not needed it should be removed. If it's needed for some reason, why not just define it as %{version} since they're the same? Saves updating it twice unless they diverge. * SOURCE should not be upper-case. Fedora packages usually go with Source0, but that's just a style note. * Defining `name`, `version` and `release` then just using them in `Name:`, `Version:` and `Release:` is a bit odd. If you just fill those fields in normally - `Name: pylero`, `Version: `0.0.3`, `Release: 1` - then %name, %version and %release get defined for you. These kinds of 'define blocks' ahead of the start of the spec are usually used to set things that aren't part of the core RPM spec, like %github_version and stuff. * `1` is wrong for the release value, anyway. It needs to be `1%{?dist}`. * For a new package, SPDX-style License: is recommended now. See https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/message/6GKXXE7AMBB76IBRMCTU3L57GLCCZL55/ for more on this. * `Group:` and `Vendor:` are not used by Fedora and should not be in the spec. * I don't think there's any need to set `Prefix:` either, though the guidelines don't explicitly address that. * The Python guidelines say "Packages MUST use the automatic Python run-time dependency generator...Dependencies covered by the generators SHOULD NOT be repeated in the .spec file". Check if the dependency generator picks up suds and click; if so, those requirements aren't needed. The requirement for `python3-%{name} == %{version}-%{release}` is probably fine if they really need a strict version tie like that. * I see upstream has tests, but you aren't running them in this package build, you should run upstream tests during package build if at all possible. I'd think the buildrequires on suds and click wouldn't actually be necessary if you're not running the tests, too - but don't take out the buildrequires, add the tests. :D This is a much bigger point and not required to pass review, but I'd recommend looking at the more modern Python macros and example spec in the guidelines: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_empty_spec_file . The automatic build dependency and pyproject-based build/install/check stuff is cool for modern-style Python projects that use the improved approaches to packaging (in the upstream sense) from the last decade or so. On a quick look at upstream it doesn't seem to have been updated for a lot of the more modern Python packaging stuff, it's still pretty old-school (setup.py, no pyproject.toml). If you're interested in updating the upstream project, then for a guide to some of that stuff, see https://hynek.me/articles/sharing-your-labor-of-love-pypi-quick-and-dirty/ , though it's aging a bit, and https://realpython.com/pypi-publish-python-package/ , which I just found but looks good. It also has a good list of links to the relevant PEPs. I would add to those that using setuptools_scm to determine package content from which files are tracked by git - instead of having to do it separately - is convenient, and makes it easy to include tests in tarballs, which you really should do so they can be run in downstream package builds like this one ;). Only issue with that is it'll get dragged into package builds unnecessarily if you use the automatic build requirements generator, see https://src.fedoraproject.org/rpms/fedfind/blob/rawhide/f/fedfind.spec#_36 for how I work around that.
(In reply to Adam Williamson from comment #1) > I'm not a sponsor so cannot do the official review, but a couple of > unofficial notes: > > * I'd be concerned about `%define _unpackaged_files_terminate_build 0`. I > can't think of many cases where that's a good idea for a Fedora spec. Why is > it here? What packages are showing up as 'unpackaged'? Is it because of the > `%exclude %{_bindir}/%{name}` - you don't want that file packaged, for some > reason? I would usually handle that by just deleting it at the end of > `%install`, which avoids the need for `_unpackaged_files_terminate_build` > and a `%exclude`. Yeah, I want to exclude one file for the rpm release, without the `%exclude` it will fail build with: Checking for unpackaged file(s): /usr/lib/rpm/check-files /home/waynesun/rpmbuild/BUILDROOT/pylero-0.0.3-1.fc36.x86_64 error: Installed (but unpackaged) file(s) found: /usr/bin/pylero RPM build errors: Installed (but unpackaged) file(s) found: /usr/bin/pylero > * Why is unmangled_version defined but never used? Does a macro use it? I > don't know that any of the standard Python macros do. If it's not needed it > should be removed. If it's needed for some reason, why not just define it as > %{version} since they're the same? Saves updating it twice unless they > diverge. > * SOURCE should not be upper-case. Fedora packages usually go with Source0, > but that's just a style note. > * Defining `name`, `version` and `release` then just using them in `Name:`, > `Version:` and `Release:` is a bit odd. If you just fill those fields in > normally - `Name: pylero`, `Version: `0.0.3`, `Release: 1` - then %name, > %version and %release get defined for you. These kinds of 'define blocks' > ahead of the start of the spec are usually used to set things that aren't > part of the core RPM spec, like %github_version and stuff. > * `1` is wrong for the release value, anyway. It needs to be `1%{?dist}`. I'll update this four parts. > * For a new package, SPDX-style License: is recommended now. See > https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/ > message/6GKXXE7AMBB76IBRMCTU3L57GLCCZL55/ for more on this. The project is using the MIT license and listed in the spec which satisfied with the SPDX license expression. > * `Group:` and `Vendor:` are not used by Fedora and should not be in the > spec. > * I don't think there's any need to set `Prefix:` either, though the > guidelines don't explicitly address that. > * The Python guidelines say "Packages MUST use the automatic Python run-time > dependency generator...Dependencies covered by the generators SHOULD NOT be > repeated in the .spec file". Check if the dependency generator picks up suds > and click; if so, those requirements aren't needed. The requirement for > `python3-%{name} == %{version}-%{release}` is probably fine if they really > need a strict version tie like that. I'll update this three parts. > * I see upstream has tests, but you aren't running them in this package > build, you should run upstream tests during package build if at all > possible. I'd think the buildrequires on suds and click wouldn't actually be > necessary if you're not running the tests, too - but don't take out the > buildrequires, add the tests. :D The test in the upstream project have issue with running as it's not unit test so I have to exclude them, I'll try remove the buildrequires for suds and click. > > > > This is a much bigger point and not required to pass review, but I'd > recommend looking at the more modern Python macros and example spec in the > guidelines: > https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/ > #_empty_spec_file . The automatic build dependency and pyproject-based > build/install/check stuff is cool for modern-style Python projects that use > the improved approaches to packaging (in the upstream sense) from the last > decade or so. I started with the new modern-style spec first but have to downgrade to 201x python rpm build spec as the modern-style does not work on epel8 which is a release target. > > On a quick look at upstream it doesn't seem to have been updated for a lot > of the more modern Python packaging stuff, it's still pretty old-school > (setup.py, no pyproject.toml). If you're interested in updating the upstream > project, then for a guide to some of that stuff, see > https://hynek.me/articles/sharing-your-labor-of-love-pypi-quick-and-dirty/ , > though it's aging a bit, and > https://realpython.com/pypi-publish-python-package/ , which I just found but > looks good. It also has a good list of links to the relevant PEPs. I would > add to those that using setuptools_scm to determine package content from > which files are tracked by git - instead of having to do it separately - is > convenient, and makes it easy to include tests in tarballs, which you really > should do so they can be run in downstream package builds like this one ;). > Only issue with that is it'll get dragged into package builds unnecessarily > if you use the automatic build requirements generator, see > https://src.fedoraproject.org/rpms/fedfind/blob/rawhide/f/fedfind.spec#_36 > for how I work around that. I'll check more on this but the epel8 still a blocker to upgrade the spec. Thanks for the review
Without the `_unpackaged_files_terminate_build` set as 0, it will fail on fedora > 36 (37 and rawhide) with: Requires: /usr/bin/python3 python(abi) = 3.11 python3.11dist(click) python3.11dist(suds) Checking for unpackaged file(s): /usr/lib/rpm/check-files /builddir/build/BUILDROOT/pylero-0.0.3-1.fc38.x86_64 error: Installed (but unpackaged) file(s) found: /usr/pylero.cfg Installed (but unpackaged) file(s) found: /usr/pylero.cfg details in: https://download.copr.fedorainfracloud.org/results/waynesun20/pylero/fedora-rawhide-x86_64/04813040-pylero/builder-live.log.gz so I have to add it back
In general, when you get an error like that, you should ask "why is a file I don't want getting installed?" and fix that, rather than just telling RPM to ignore it. Why is the upstream install stuff installing a file `/usr/pylero.cfg` in the first place? That's a very weird thing to install. To me it would indicate something's wrong with the upstream installer scripts. But I'd also be wondering, is there meant to be a sample or real config file installed? If so, how do I get it installed to the right place, and is it appropriate to package it? If you're sure you don't need the file and it being there isn't an indicator of something more seriously wrong, then as I said, instead of specifying `_unpackaged_files_terminate_build`, I would wipe the file at the end of %install , e.g.: %install %py3_install rm -f %{buildroot}{%_bindir}/pylero to get rid of the executable you don't want to ship.
The unwanted pylero cli file have already been removed as you suggested within the %install in the cfg and it is still required to be added to the `%exclude` to work. The pylero.cfg is the user config sample file included in MANIFEST.in and data_files in setup.py to get it in. I struggled to keep the file in case of user complains as the old way have it. With remove it from the data_files it'll not be included in the rpm package. I don't find another way than the add in the current data_files with the `_unpackaged_files_terminate_build` config. I feel this is to much to maintain, I'll remove the data_file without include that user config and update user doc with this change.
FWIW, I like using setuptools_scm to control the contents of upstream tarball builds rather than a manually-maintained MANIFEST.in, and I would think that would let you include the file in the tarball without including it in installs...
On the setuptools_scm repo https://github.com/pypa/setuptools_scm It mentioned: This feature requires Setuptools 42 or later, released in Nov, 2019. while on EPEL8 the setuptools are 39, so it's not viable. And use setup.py with setuptools_scm is deprecated, so I'll need to wait on update the project with `pyproject.toml` modern package format with modern rpm build spec I have removed the data_files and use the `include_package_data=True` with update the sample config file path which fixed the issue with including package data with setting `_unpackaged_files_terminate_build`. Also have fixed the other issues mentioned in previous comment and updated the review description with the new urls.
When you use setuptools_scm, that's all upstream, nothing in the downstream package build involves it. (In fact one 'fun' thing is you have to patch setuptools_scm out of the build deps if you use automatic build dep generation, heh). Downstream package build starts from the source tarball, after all, it doesn't involve building it. It's possible and quite common for projects to have both a pyproject.toml and a setup.py. All my projects do. For instance, fedfind - https://pagure.io/fedora-qa/fedfind - configures several things in pyproject.toml , but also still has a setup.py. I build that package on everything from epel7 onwards; the Fedora specs uses the shiny new macros: https://src.fedoraproject.org/rpms/fedfind/blob/rawhide/f/fedfind.spec https://src.fedoraproject.org/rpms/fedfind/blob/f36/f/fedfind.spec etc. (you can see the setuptools_scm dep workaround there...), but the EPEL specs are still old-school: https://src.fedoraproject.org/rpms/fedfind/blob/epel7/f/fedfind.spec https://src.fedoraproject.org/rpms/fedfind/blob/epel8/f/fedfind.spec and that all works OK. At some point I might decide to ditch setup.py and do everything in pyproject.toml and maroon epel7 and epel8 on an older release branch or something, but I haven't gone that far yet.
(In reply to Adam Williamson from comment #8) > When you use setuptools_scm, that's all upstream, nothing in the downstream > package build involves it. (In fact one 'fun' thing is you have to patch > setuptools_scm out of the build deps if you use automatic build dep > generation, heh). Downstream package build starts from the source tarball, > after all, it doesn't involve building it. I have updated Pylero upstream project using both pyproject.toml and setup.py now, which is for modern pypi package build interface. > > It's possible and quite common for projects to have both a pyproject.toml > and a setup.py. All my projects do. For instance, fedfind - > https://pagure.io/fedora-qa/fedfind - configures several things in > pyproject.toml , but also still has a setup.py. I build that package on > everything from epel7 onwards; the Fedora specs uses the shiny new macros: > > https://src.fedoraproject.org/rpms/fedfind/blob/rawhide/f/fedfind.spec > https://src.fedoraproject.org/rpms/fedfind/blob/f36/f/fedfind.spec > etc. > > (you can see the setuptools_scm dep workaround there...), but the EPEL specs > are still old-school: > > https://src.fedoraproject.org/rpms/fedfind/blob/epel7/f/fedfind.spec > https://src.fedoraproject.org/rpms/fedfind/blob/epel8/f/fedfind.spec > > and that all works OK. At some point I might decide to ditch setup.py and do > everything in pyproject.toml and maroon epel7 and epel8 on an older release > branch or something, but I haven't gone that far yet. I have moved the upstream project back on modern rpm spec, which I have updated the link in bug description. The 201x era spec will only be used in the epel8 downstream repo as your repo. Thanks for the help and recommendations on this.
Guannan (Wayne Sun) emailed me and asked me to be their sponsor. I am taking this review and will submit detailed feedback shortly. Thank you Adam for the existing comments, I'll make sure to read the entire conversation.
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."""
> I understand from the RUL that this spec file lives in the upstream repository s/RUL/URL/
Writing this to make fedora-review download the latest known SRPM: Spec URL: https://raw.githubusercontent.com/RedHatQE/pylero/main/pylero.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/waynesun20/pylero/fedora-rawhide-x86_64/04868824-pylero/python3-pylero-0.0.4-1.fc38.noarch.rpm
I almost did not notice the component name. 9) [MUST] If this is primarily an application, the package name (both the built package and component name) MUST be pylero, not pylero component and python3-pylero subpackage. If this is primarily a Python library, the component name MUST be python-pylero and the subpackage name must be python3-pylero. If the primary purpose is not clear -- i.e. this is both an application and a library -- you choose the one you prefer and stick to it. If the binary package is going to be called pylero, you can add %py_provides python3-pylero. If the binary package is going to be called python3-pylero, you can add Provides pylero = %{version}-%{release}. The current setup (component pylero, binary package python3-pylero) is incorrect. See https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_naming -- namely https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_library_naming and https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_application_naming
(In reply to Miro Hrončok from comment #13) > Writing this to make fedora-review download the latest known SRPM... Correction: Spec URL: https://raw.githubusercontent.com/RedHatQE/pylero/main/pylero.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/waynesun20/pylero/fedora-rawhide-x86_64/04868824-pylero/pylero-0.0.4-1.fc38.src.rpm
Miro: on point 5, it's been a while since I started just putting this in all the specs I maintain for things that use this pattern, so I don't recall for sure. I thought it did cause an actual build failure, but I just tried commenting it out of one spec I use it in and doing scratch builds for F35 and Rawhide and they worked fine, and the resulting packages don't have a runtime dependency on setuptools_scm. setuptools_scm isn't packaged for EPEL, either, but of course that doesn't really matter as you can't use the %pyproject stuff on EPEL anyway. So, maybe it's not really important as you said.
> setuptools_scm isn't packaged for EPEL, either, but of course that doesn't really matter as you can't use the %pyproject stuff on EPEL anyway. python3-setuptools_scm is available in RHEL 9 (CRB) and RHEL 8 (Powertools) as well as EPEL 7 (python36-setuptools_scm and even python34-setuptools_scm). It is quite an essential package.
ah, okay, I was kinda assuming it'd be in EPEL if anywhere. Good to know.
10) [MUST] "at least a basic smoke test (such as importing the packaged module) MUST be run in %check" https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_tests However I see that upstream has proper tests, so you SHOULD try to run those if at all possible. "If a test suite exists upstream, it SHOULD be run in the %check section" (same guideline as the one quoted above).
We covered that in the initial few comments. Wayne said "The test in the upstream project have issue with running as it's not unit test so I have to exclude them, I'll try remove the buildrequires for suds and click."
In that case, don't remove the buildrequires for suds and click and run at least the import check https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#pyproject_check_import
(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.
Sorry for the delay, I somehow missed this is back on me now. > 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. I don't know what you mean here, sorry. Does the command not work when executed? > I have updated the package name to python-pylero as it's preferred to be used as library. OK. Please rename it to python-pylero.spec and post new links to spec and srpm. Always post new links to spec and srpm when you make updates to a package on review. Always in this form: Spec URL: https://... SRPM URL: https://... Make sure the links go to the actual spec and srpm (e.g. use the "raw" links when on GitHub). > I have added the %pyproject_check_import under %check. Thanks. Why with -t?
(In reply to Miro Hrončok from comment #23) > Sorry for the delay, I somehow missed this is back on me now. > > > > > 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. > > I don't know what you mean here, sorry. Does the command not work when > executed? > It works but it works better when using gnureadline package. > > > I have added the %pyproject_check_import under %check. > > Thanks. Why with -t? If the Polarion server url is not specified in config some library will failed to import, so only specify to import from top.
Updated spec urls Spec URL: https://raw.githubusercontent.com/RedHatQE/pylero/main/python-pylero.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/waynesun20/pylero/fedora-rawhide-x86_64/04893370-python-pylero/python-pylero-0.0.4-1.fc38.src.rpm
(In reply to Wayne Sun from comment #24) > (In reply to Miro Hrončok from comment #23) > > Sorry for the delay, I somehow missed this is back on me now. > > > > > > > > > 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. > > > > I don't know what you mean here, sorry. Does the command not work when > > executed? > > > > It works but it works better when using gnureadline package. Ack. > > > I have added the %pyproject_check_import under %check. > > > > Thanks. Why with -t? > > If the Polarion server url is not specified in config some library will > failed to import, so only specify to import from top. Every time something like this is necessary it is a really good idea to put a comment in the spec, so your future self or another future maintainer knows. # If the Polarion server URL is not specified in config some libraries will # fail to import, so only specify to import the top-level module: %pyproject_check_import -t Is it possible to exclude the failing libraries with -e instead or would the list be too long?
(In reply to Miro Hrončok from comment #26) > > > Every time something like this is necessary it is a really good idea to put > a comment in the spec, so your future self or another future maintainer > knows. > > # If the Polarion server URL is not specified in config some libraries will > # fail to import, so only specify to import the top-level module: > %pyproject_check_import -t > ACK > > Is it possible to exclude the failing libraries with -e instead or would the > list be too long? With -e the line will be: %pyproject_check_import -e pylero.build -e pylero.build_linked_work_item -e pylero.cli.cmd -e pylero.document -e pylero.plan -e pylero.plan_record -e pylero.test_record -e pylero.test_run -e pylero.work_item not that long but better switch lines, how about: %global _pylero_import_exclude %{expand: -e pylero.build -e pylero.build_linked_work_item -e pylero.cli.cmd -e pylero.document -e pylero.plan -e pylero.plan_record -e pylero.test_record -e pylero.test_run -e pylero.work_item } %check %pyproject_check_import %_pylero_import_exclude
Whether or not you go with %pyproject_check_import -t or -e is up to you. Ideally, you make the actual tests work. Packages that are not tested in %check are much more likely to be broken without notice. You might also want to run the CLI tools, e.g. with --help, in %check. I've tried to run the package and it's rather unusual that it tracebacks immediately without config: $ pylero --help Traceback (most recent call last): File "/usr/bin/pylero", line 39, in <module> main() File "/usr/bin/pylero", line 30, in main imp_mod = __import__("pylero.{0}".format(the_mod), fromlist=[""]) ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/pylero/build.py", line 8, in <module> from pylero.build_linked_work_item import ArrayOfBuildLinkedWorkItem File "/usr/lib/python3.11/site-packages/pylero/build_linked_work_item.py", line 9, in <module> from pylero.work_item import _WorkItem File "/usr/lib/python3.11/site-packages/pylero/work_item.py", line 1580, in <module> cfg = Configuration() ^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/pylero/base_polarion.py", line 105, in __init__ raise PyleroLibException( pylero.exceptions.PyleroLibException: The config files must contain valid values for: url, user, password and default_project $ pylero-cmd --help Traceback (most recent call last): File "/usr/bin/pylero-cmd", line 3, in <module> from pylero.cli.cmd import CmdList File "/usr/lib/python3.11/site-packages/pylero/cli/cmd.py", line 9, in <module> from pylero.document import Document File "/usr/lib/python3.11/site-packages/pylero/document.py", line 24, in <module> from pylero.work_item import _WorkItem File "/usr/lib/python3.11/site-packages/pylero/work_item.py", line 1580, in <module> cfg = Configuration() ^^^^^^^^^^^^^^^ File "/usr/lib/python3.11/site-packages/pylero/base_polarion.py", line 105, in __init__ raise PyleroLibException( pylero.exceptions.PyleroLibException: The config files must contain valid values for: url, user, password and default_project Together with a lack of a manpage, it's a bit hard to understand how to provide the config. It's in the README, but I'd expect --help to work. I would be reluctant to ship this to our users in this form, but OTOH assessing the friendliness of packaged software is not up to the package reviewer. The packaging itself is quite alright otherwise: Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated Issues: ======= The CLI immediately crashes. Considering you are the upstream, I recommend adding manual pages there. But not a blocker. ===== MUST items ===== Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [x]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "MIT License", "*No copyright* MIT License". [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [-]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 20480 bytes in 1 files. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Packages must not store files under /srv, /opt or /usr/local Python: [x]: Python eggs must not download any dependencies during the build process. [x]: A package which is used by another package via an egg interface should provide egg info. [x]: Package meets the Packaging Guidelines::Python [x]: Package contains BR: python2-devel or python3-devel [x]: Packages MUST NOT have dependencies (either build-time or runtime) on packages named with the unversioned python- prefix unless no properly versioned package exists. Dependencies on Python packages instead MUST use names beginning with python2- or python3- as appropriate. [x]: Python packages must not contain %{pythonX_site(lib|arch)}/* in %files [x]: Binary eggs must be removed in %prep ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [?]: Package functions as described. [?]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [?]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [?]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- python3-pylero.noarch: W: no-manual-page-for-binary pylero python3-pylero.noarch: W: no-manual-page-for-binary pylero-cmd 2 packages and 1 specfiles checked; 0 errors, 2 warnings, 0 badness; has taken 1.3 s Considering you are the upstream, I recommend adding manual pages there. But not a blocker. Rpmlint (installed packages) ---------------------------- python3-pylero.noarch: W: no-manual-page-for-binary pylero python3-pylero.noarch: W: no-manual-page-for-binary pylero-cmd 1 packages and 0 specfiles checked; 0 errors, 2 warnings, 0 badness; has taken 0.1 s Source checksums ---------------- https://github.com/RedHatQE/pylero/archive/0.0.4/pylero-0.0.4.tar.gz : CHECKSUM(SHA256) this package : 40d4b5c984a13fd654c2b81afe23babffa3146542c525181635a0ed2c1b4e217 CHECKSUM(SHA256) upstream package : 40d4b5c984a13fd654c2b81afe23babffa3146542c525181635a0ed2c1b4e217 BuildRequires ------------- (python3dist(toml) if python3-devel < 3.11) pyproject-rpm-macros python3-devel python3dist(click) python3dist(packaging) python3dist(pip) >= 19 python3dist(setuptools) >= 40.8 python3dist(setuptools) >= 45 python3dist(setuptools-scm) python3dist(suds) python3dist(wheel) It's a bit weird that this requires 2 different setuptools minimal versions, but in practice, it works. Requires -------- python3-pylero (rpmlib, GLIBC filtered): /usr/bin/python3 python(abi) = 3.11 python3.11dist(click) python3.11dist(suds) Provides -------- python3-pylero: python-pylero = 0.0.4-1.fc38 python3-pylero = 0.0.4-1.fc38 python3.11-pylero = 0.0.4-1.fc38 python3.11dist(pylero) = 0.0.4 python3dist(pylero) = 0.0.4 Generated by fedora-review 0.7.0 (fed5495) last change: 2019-03-17 Command line :try-fedora-review -b 2115102 -m fedora-rawhide-x86_64 Buildroot used: fedora-rawhide-x86_64 Active plugins: Shell-api, Generic, Python Disabled plugins: C/C++, SugarActivity, Perl, Haskell, Ruby, fonts, R, PHP, Java, Ocaml Disabled flags: EPEL6, EPEL7, DISTTAG, BATCH, EXARCH
Any update here?
(In reply to Jenny Severance from comment #29) > Any update here? I'm working on becoming a packager which need be sponsored, Miro Hrončok is helping on that. I'm finally starting to do informal review for several other package review bugs which is part of the process to get familiar with packaging and become a packager, already got two approved, until been sponsored I can't make the package into fedora.
(In reply to Wayne Sun from comment #30) > (In reply to Jenny Severance from comment #29) > > Any update here? > > I'm working on becoming a packager which need be sponsored, Miro Hrončok is > helping on that. > > I'm finally starting to do informal review for several other package review > bugs which is part of the process to get familiar with packaging and become > a packager, already got two approved, until been sponsored I can't make the > package into fedora. correction: already help 3 packages review approved
(In reply to Wayne Sun from comment #31) > (In reply to Wayne Sun from comment #30) > > (In reply to Jenny Severance from comment #29) > > > Any update here? > > > > I'm working on becoming a packager which need be sponsored, Miro Hrončok is > > helping on that. > > > > I'm finally starting to do informal review for several other package review > > bugs which is part of the process to get familiar with packaging and become > > a packager, already got two approved, until been sponsored I can't make the > > package into fedora. Ack thank you the update, Wayne. A lot of checks and balances in place, which is a good thing! > > correction: already help 3 packages review approved
This package is APPROVED and Wayne was sponsored.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/python-pylero
FEDORA-2022-fca2129c56 has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-fca2129c56
FEDORA-EPEL-2022-d2c16fa392 has been submitted as an update to Fedora EPEL 9. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-d2c16fa392
FEDORA-2022-6597623449 has been pushed to the Fedora 37 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-6597623449 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-6597623449 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-fca2129c56 has been pushed to the Fedora 36 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2022-fca2129c56 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-fca2129c56 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-EPEL-2022-d2c16fa392 has been pushed to the Fedora EPEL 9 testing repository. You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2022-d2c16fa392 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-fca2129c56 has been pushed to the Fedora 36 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-6597623449 has been pushed to the Fedora 37 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-EPEL-2022-d2c16fa392 has been pushed to the Fedora EPEL 9 stable repository. If problem still persists, please make note of it in this bug report.