Spec URL: https://wojnilowicz.fedorapeople.org/python-aw-client.spec SRPM URL: https://wojnilowicz.fedorapeople.org/python-aw-client-0.5.13-1.fc39.src.rpm Description: Client library for ActivityWatch in Python. Fedora Account System Username: wojnilowicz
Copr build: https://copr.fedorainfracloud.org/coprs/build/7597082 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2291346-python-aw-client/fedora-rawhide-x86_64/07597082-python-aw-client/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Some remarks: Are the sed commands necessary? ``` sed -r -i '/^[[:blank:]]*pytest-cov\b/d' pyproject.toml sed -r -i '/^[[:blank:]]*pylint\b/d' pyproject.toml sed -r -i '/^[[:blank:]]*black\b/d' pyproject.toml ``` These seem to be used only in dev dependency, but you manually add `pytest` to it. Otherwise types-* should also be excluded, along with everything else frankly. I'm not a fan of `%{srcname}`, but only `%{_bindir}/%{srcname}` in particular should be changed because there is generally no connection between those. Can you explain the choice for using the git commit and/or link to some discussion upstream on it? --- Other than that LGTM. Would have preferred more tests to be run since the available tests are quite barren, even if they would need to be run outside of package build, but the `input()` would be a bit tricky to deal with.
(In reply to Cristian Le from comment #2) > Some remarks: > > Are the sed commands necessary? > ``` > sed -r -i '/^[[:blank:]]*pytest-cov\b/d' pyproject.toml > sed -r -i '/^[[:blank:]]*pylint\b/d' pyproject.toml > sed -r -i '/^[[:blank:]]*black\b/d' pyproject.toml > ``` > These seem to be used only in dev dependency, but you manually add `pytest` > to it. Otherwise types-* should also be excluded, along with everything else > frankly. Actually not. As far as I can see no linter is run in %check when I removed those lines. I've updated the .spec file > I'm not a fan of `%{srcname}`, but only `%{_bindir}/%{srcname}` in > particular should be changed because there is generally no connection > between those. Replaced `%{_bindir}/%{srcname}` with `%{_bindir}/aw-client`. > Can you explain the choice for using the git commit and/or link to some > discussion upstream on it? A GitHub release is stuck at 0.5.9 but the source code is already at 0.5.13, so I choosen to follow git. > --- > > Other than that LGTM. Would have preferred more tests to be run since the > available tests are quite barren, even if they would need to be run outside > of package build, but the `input()` would be a bit tricky to deal with. Not sure if that would be possible. At first look the tests in this package require aw-server which I'm not going to even package.
> A GitHub release is stuck at 0.5.9 but the source code is already at 0.5.13, so I choosen to follow git. Tags for `0.5.13` are there, and pypi source [1] has the release also. Can you change to pypi_source? > Not sure if that would be possible. At first look the tests in this package require aw-server which I'm not going to even package. Fair point. I would have thought `aw-server-rust` would still work for that [1]: https://pypi.org/project/aw-client/#files
(In reply to Cristian Le from comment #4) > > A GitHub release is stuck at 0.5.9 but the source code is already at 0.5.13, so I choosen to follow git. > > Tags for `0.5.13` are there, and pypi source [1] has the release also. Can > you change to pypi_source? Not really. The guide at https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_files_from_pypi mentions PyPi packages as a possibility and not as a first choice. For me it would mean extra work for no advantage. Why would you like that? As for the tags, they are there but no zip releases. Following a git seems to be most flexible, especially that other packages from this author can be tracked by git commit only. > > Not sure if that would be possible. At first look the tests in this package require aw-server which I'm not going to even package. > > Fair point. I would have thought `aw-server-rust` would still work for that > > [1]: https://pypi.org/project/aw-client/#files I'm not sure. I've never did that. I don't want to go down on strange errors because the author tested it on aw-server and I did that on aw-server-rust and something doesn't pass the test.
> For me it would mean extra work for no advantage. Why would you like that? If the project moves to dynamic-version, the PyPI source is guaranteed to work. Currently EPEL9 does not work with dynamic versioning even when `.git_archival.txt` is included appropriately. > As for the tags, they are there but no zip releases. Following a git seems to be most flexible, especially that other packages from this author can be tracked by git commit only. You have git archives, e.g.: https://github.com/ActivityWatch/aw-client/releases/tag/v0.5.13 If you want it to be more flexible, than how about forge_meta [1]? > I'm not sure. I've never did that. I don't want to go down on strange errors because the author tested it on aw-server and I did that on aw-server-rust and something doesn't pass the test. Well even with that, the author chose to use `input()` to block the automated testing so it's rather hard to implement regardless. Hopefully there are other projects that can do integration tests for it, or just cross_fingers and test live on users :D. [1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/
(In reply to Cristian Le from comment #6) > > For me it would mean extra work for no advantage. Why would you like that? > > If the project moves to dynamic-version, the PyPI source is guaranteed to > work. Currently EPEL9 does not work with dynamic versioning even when > `.git_archival.txt` is included appropriately. I don't target EPEL9. Do you have any source of that information? At https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#_handling_non_sorting_versions_with_tilde_dot_and_caret I can only find that it wouldn't be compatible with EPEL7. > > As for the tags, they are there but no zip releases. Following a git seems to be most flexible, especially that other packages from this author can be tracked by git commit only. > > You have git archives, e.g.: > https://github.com/ActivityWatch/aw-client/releases/tag/v0.5.13 Oh. I didn't know it. Thanks. On one hand it would be good to have 0.5.13 but on the other there are some fixes like this one https://github.com/ActivityWatch/aw-client/commit/7e12bf2c9727b6abba01e36669c04ebc69b5c89a that appear at random across whole ActivityWatch and might not be included in the last point release. If you don't mind, I would stick with git. > If you want it to be more flexible, than how about forge_meta [1]? I don't know forge_meta. Git seems to be easy enough and flexible for me. The Packaging Guideline is also fine with this. > > I'm not sure. I've never did that. I don't want to go down on strange errors because the author tested it on aw-server and I did that on aw-server-rust and something doesn't pass the test. > > Well even with that, the author chose to use `input()` to block the > automated testing so it's rather hard to implement regardless. Hopefully > there are other projects that can do integration tests for it, or just > cross_fingers and test live on users :D. > > [1]: https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/ Oh. I see. Rocket crates are no better. They're not cut clean for testing.
> I don't target EPEL9. Do you have any source of that information? Personal experience [1,2]. But it's more of cautionary that there can be subtle differences in the git archive vs pypi_source. > Oh. I didn't know it. Thanks. On one hand it would be good to have 0.5.13 but on the other there are some fixes like this one Ideally the relevant patches are cherry-picked instead. You can easily create them by adding a `.patch` at the end of a github URL [3] (these are compatible with pypi_source btw). Would be even good to use that as a `Source` url also, but put in a readable name for it if you do that way. This approach balances better the intention of upstream to distribute a stable release and the need to backport some relevant fixes (the example commit shown only affected the examples which are not being packaged or used in the testing afaik) [1]: https://github.com/teemtee/fmf/pull/202#discussion_r1611665924 [2]: https://copr.fedorainfracloud.org/coprs/packit/teemtee-fmf-202/build/7481277/ [3]: https://github.com/ActivityWatch/aw-client/commit/7e12bf2c9727b6abba01e36669c04ebc69b5c89a.patch
(In reply to Cristian Le from comment #8) > > I don't target EPEL9. Do you have any source of that information? > > Personal experience [1,2]. It seems to me that you're referring to a deprecated versioning scheme described at https://docs.fedoraproject.org/en-US/packaging-guidelines/Versioning/#traditional-versioning and recommended for shipping complex versioning to EPEL7. From brief reading, I'm not even sure, you're using it as prescribed. Besides, none of this is my target, so I would rather not bring that up here. The versioning scheme I use is not deprecated, and as you can see at https://src.fedoraproject.org/rpms/loguru works for EPEL9. > But it's more of cautionary that there can be > subtle differences in the git archive vs pypi_source. As mentioned earlier, "PyPi packages as a possibility and not as a first choice.". I believe, I follow the Packaging Guideline and expect you to follow it as well when reviewing. > > Oh. I didn't know it. Thanks. On one hand it would be good to have 0.5.13 but on the other there are some fixes like this one > > Ideally the relevant patches are cherry-picked instead. You can easily > create them by adding a `.patch` at the end of a github URL [3] (these are > compatible with pypi_source btw). Would be even good to use that as a > `Source` url also, but put in a readable name for it if you do that way. > This approach balances better the intention of upstream to distribute a > stable release and the need to backport some relevant fixes Can you provide any source in the Packaging Guidelines that it should be done as you describe it? > (the example > commit shown only affected the examples which are not being packaged or used > in the testing afaik) Fair enough. I didn't analyze the content of this patch. > [3]: https://github.com/ActivityWatch/aw-client/commit/7e12bf2c9727b6abba01e36669c04ebc69b5c89a.patch That's another nice feature of GitHub that I did not know about. Thanks for sharing. Anyway I believe, I'm using a valid sourcing method as seen at https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_using_revision_control Do you really want me to switch to the 0.5.13 point release?
(In reply to wojnilowicz from comment #9) > (In reply to Cristian Le from comment #8) > > > I don't target EPEL9. Do you have any source of that information? > > > > Personal experience [1,2]. > > It seems to me that you're referring to a deprecated versioning scheme > described at It's not that, it's related to `setuptools_scm` not working properly on EPEL9. It doesn't apply in this case, but I mainly mention this because there can be subtle differences, which can help minimizes surprises down the line. Ultimately as you will be maintaining it and have more experience with the project and ecosystem, I will leave it to your call which source to use. --- One thing I will push back on is the use of commit for packaging. It is not mentioned clearly in the packaging guidelines, but it is partially covered in the review template ===== SHOULD items ===== Generic: [ ]: Latest version is packaged. In the case of `loguru` that you have pointed, it is more justified because: - It is a C++ project so there are no standard package repos like PyPI to consider - The package was initially submitted for review in 2024, but there are no tags/releases since 2019, despite an open issue of that [1], despite a major feature of CMake support being added [2] In this project, the release schedule seems relatively healthy, it is still in active development, and I don't see an issue requesting tagging a new version. Comparing the changes to the main branch [3], I don't see any relevant commits that would justify not using the published version. --- I understand that you have reservations on this since it is not formally stated in the packaging guidelines, so by all means, ask for a second opinion. And if you find the packaging guidelines are a bit ambiguous on certain topics, feel free to raise issues and suggestions on it (top-right corner on a page). You can see that these guidelines are constantly evolving. [1]: https://github.com/emilk/loguru/issues/239 [2]: https://github.com/emilk/loguru/pull/215 [3]: https://github.com/ActivityWatch/aw-client/compare/v0.5.13...master
Will you approve the package if I switch to 0.5.13?
Yes, that is the only remaining real review comment that I had, everything else was mentioned in comment #3
I've switched. [fedora-review-service-build]
There seems to be some problem with the following file. SRPM URL: https://wojnilowicz.fedorapeople.org/python-aw-client-0.5.13-1.fc39.src.rpm Fetching it results in a 403 Forbidden error. Please make sure the URL is correct and publicly available. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
[fedora-review-service-build]
Created attachment 2037963 [details] The .spec file difference from Copr build 7597082 to 7648041
Copr build: https://copr.fedorainfracloud.org/coprs/build/7648041 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2291346-python-aw-client/fedora-rawhide-x86_64/07648041-python-aw-client/fedora-review/review.txt Please take a look if any issues were found. --- This comment was created by the fedora-review-service https://github.com/FrostyX/fedora-review-service If you want to trigger a new Copr build, add a comment containing new Spec and SRPM URLs or [fedora-review-service-build] string.
Package approved --- Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== MUST items ===== Generic: [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. Note: Using prebuilt packages [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: "*No copyright* Mozilla Public License 2.0", "Unknown or generated". 28 files have unknown license. Detailed output of licensecheck in /var/lib/copr-rpmbuild/results/python-aw- client/licensecheck.txt [x]: Package requires other packages for directories it uses. Note: No known owner of /usr/lib/python3.13/site-packages, /usr/share/doc, /usr/lib/python3.13, /usr/share/licenses, /usr/bin, /usr/lib, /usr, /usr/share [x]: Package must own all directories that it creates. Note: Directories without known owners: /usr/lib/python3.13/site- packages, /usr/share/doc, /usr/lib/python3.13, /usr/share/licenses, /usr/bin, /usr/lib, /usr, /usr/share [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [x]: Package contains desktop file if it is a GUI application. [x]: 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. [x]: 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. [x]: Package contains systemd file(s) if in need. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: The License field must be a valid SPDX expression. [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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 3362 bytes in 1 files. [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: [x]: Reviewer should test that the package builds in mock. [x]: 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). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [x]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [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.
The Pagure repository was created at https://src.fedoraproject.org/rpms/python-aw-client
FEDORA-2024-2f32328a0f (python-aw-client-0.5.13-1.fc41) has been submitted as an update to Fedora 41. https://bodhi.fedoraproject.org/updates/FEDORA-2024-2f32328a0f
FEDORA-2024-2f32328a0f (python-aw-client-0.5.13-1.fc41) has been pushed to the Fedora 41 stable repository. If problem still persists, please make note of it in this bug report.
Thanks, but please don't assign yourself as a reviewer to my packages anymore.