Spec URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03247435-python-teamcity-messages/python-teamcity-messages.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03247435-python-teamcity-messages/python-teamcity-messages-1.30-1.fc36.src.rpm Description: This package integrates Python with the TeamCity <http://www.jetbrains.com/teamcity/> Continuous Integration (CI) server. It allows sending "service messages" <https://confluence.jetbrains.com/display/TCDL/Build+Script+Interaction+with+TeamCity> from Python code. Additionally, it provides integration with the following testing frameworks and tools: - py.test <http://pytest.org/> - nose <https://nose.readthedocs.org/> - Django <https://docs.djangoproject.com/en/1.8/topics/testing/advanced/#other-testing-frameworks> - unittest (Python standard library) <https://docs.python.org/2/library/unittest.html> - Trial (Twisted) <http://twistedmatrix.com/trac/wiki/TwistedTrial> - Flake8 <https://flake8.readthedocs.org/> - Behave <https://behave.readthedocs.io/> - PyLint <https://www.pylint.org/> Fedora Account System Username: mmassari FE-NEEDSPONSOR
Unofficial review Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated ===== Found issues ===== [!]: Package does not own files or directories owned by other packages. [!]: Rpmlint is run on all installed packages. See below for details. ===== MUST items ===== Generic: [!]: Package does not own files or directories owned by other packages. Problem: /usr/lib/python3.10/site-packages/twisted/ is owned by python3-twisted Fix: We can own only the files, not the parent directories, i.e. - %{python3_sitelib}/twisted + %{python3_sitelib}/twisted/plugins/* but that would create unowned directories if python3-twisted wasn't installed as well. Given that the twisted plugin is installed in the main package (here I mean Python, not RPM) and not for example as package extras I think we have to add 'Requires: python3dist(twisted)' to the main package (with a comment explaining why) as well even if it's only for the directory structure. [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. [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. [-]: Package is not known to require an ExcludeArch tag. [-]: Large documentation must go in a -doc subpackage. [-]: 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. [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. [?]: Package requires other packages for directories it uses. See my suggestion above. [?]: Package must own all directories that it creates. See my suggestion above. [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. [-]: 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]: 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. [?]: Final provides and requires are sane (see attachments). See my suggestion above. [?]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [-]: Description and summary sections in the package spec file contains translations for supported Non-English languages, if available. [-]: Package should compile and build into binary rpms on all supported architectures. [!]: Spec use %global instead of %define unless justified. There's "%define release 1", but it's unused, so can be removed. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [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. ===== EXTRA items ===== Generic: [x]: Spec file according to URL is the same as in SRPM. [!]: Rpmlint is run on all installed packages. Some lines (those with links) in the description are longer than 80 characters. I'd probably remove the links from the description. The package info contains URL to upstream where one can get all the links.
> I think we have to add 'Requires: python3dist(twisted)' to the main package (with a comment explaining why) as well even if it's only for the directory structure. Or you can create a python3-teamcity-messages-twisted[-plugin] subpackage, only which would require the twisted. For an example of a subpackage, see for example https://src.fedoraproject.org/rpms/bodhi/blob/rawhide/f/bodhi.spec#_133
I removed URLs from description, removed the unused define and created a subpackage for the twisted plugin. New SPEC URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03252810-python-teamcity-messages/python-teamcity-messages.spec New SRPM URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03252810-python-teamcity-messages/python-teamcity-messages-1.30-1.fc36.src.rpm Koji build: https://koji.fedoraproject.org/koji/taskinfo?taskID=81827044 The rpmlint output is like this: ============================================ rpmlint session starts =========================================== rpmlint: 2.2.0 configuration: /usr/lib/python3.10/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 ============= 0 packages and 1 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.5 s ============
Blocking FE-NEEDSPONSOR as this is Maja's first package. I will handle the final part of the review. Thou, I still appreciate any unofficial review comments.
[ ]: Package does not own files or directories owned by other packages. Note: Dirs in package are owned also by: /usr/lib/python3.10/site- packages/twisted(python3-autobahn, python3-twisted), /usr/lib/python3.10/site-packages/twisted/plugins(python3-autobahn, python3-twisted), /usr/lib/python3.10/site- packages/twisted/plugins/__pycache__(python3-twisted, python3-txtorcon, python3-autobahn, python3-carbon, python3-magic- wormhole-transit-relay, python3-magic-wormhole-mailbox-server, python3-daphne) This: %files -n python3-%{pypi_name}-twisted-plugin .... %{python3_sitelib}/twisted actually results in owning /usr/lib/python3.10/site-packages/twisted /usr/lib/python3.10/site-packages/twisted/plugins /usr/lib/python3.10/site-packages/twisted/plugins/__pycache__ /usr/lib/python3.10/site-packages/twisted/plugins/__pycache__/teamcity_plugin.cpython-310.opt-1.pyc /usr/lib/python3.10/site-packages/twisted/plugins/__pycache__/teamcity_plugin.cpython-310.pyc /usr/lib/python3.10/site-packages/twisted/plugins/teamcity_plugin.py but $ rpm -qf /usr/lib/python3.10/site-packages/twisted python3-twisted-21.7.0-3.fc35.noarch and you already require python3dist(twisted) so this directory will be always owned and presented py python3-twisted The same applies for /usr/lib/python3.10/site-packages/twisted/plugins It should be: %files -n python3-%{pypi_name}-twisted-plugin %license LICENSE.md %doc README.rst DEVGUIDE.md %doc examples %{python3_sitelib}/twisted/plugins/teamcity_plugin.py %{python3_sitelib}/twisted/plugins/__pycache__/* Otherwise the package looks good to me.
Hi all, and thanks to your reviews. Since I have had many suggestions I summarized here below what you will find in my last specfile. 1. I used %autochangelog in the %changelog section 2. For the problem related with the ownership of twisted and twisted/plugins directories, I am using the following directive: %pycached %{python3_sitelib}/twisted/plugins/teamcity_plugin.py 3. I finally got to run the unit tests using tox, patching setup.py and tox.ini. But this leads to a new problem. I do not know why but the pypi source tarball does not have the tox.ini file in the tree. So patches did not apply. I tried to use the source tarball from github, which has the tox file in it, but the link is https://github.com/JetBrains/teamcity-messages/archive/refs/tags/v1.30.tar.gz and the downloaded file is teamcity-messages-1.30.tar.gz and for this reason rpmbuild complains with the uri... I managed to build the rpm using: Source0: teamcity-messages-1.30.tar.gz But now rpmlint gave me a new warning and also the reviewer tool . Because Source0 is not a valid URL. Do you have any better ideas on how I can manage this issue? 4. Since I am not using the macro %pyproject_check_import anymore. I got rid of pyproject_wheel and pyproject_install and I am using py3_build and py3_install instead. Link to COPR build: https://copr.fedorainfracloud.org/coprs/build/3255812 Link to KOJI build: https://koji.fedoraproject.org/koji/taskinfo?taskID=82002626 New SPEC URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03255812-python-teamcity-messages/python-teamcity-messages.spec New SRPM URL: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03255812-python-teamcity-messages/python-teamcity-messages-1.30-3.fc36.src.rpm rpmlint output =============================================================================== rpmlint session starts =============================================================================== rpmlint: 2.2.0 configuration: /usr/lib/python3.10/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 python-teamcity-messages.spec: W: invalid-url Source0: teamcity-messages-1.30.tar.gz ================================================ 0 packages and 1 specfiles checked; 0 errors, 1 warnings, 0 badness; has taken 0.2 s ================================================
Ad tarball. You can either ask upstream to include tox.ini in tar ball uploaded to PyPI or use the gitlab tarball. Both are OK. See https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/#_git_tags for handling tarballs from Github.
Ad Patch0: tox.ini.patch when the patch adds the whole new file you are better with: Source1: tox.ini %prep %autosetup -n %{pypi_name}-%{version} -p1 cp -a %{SOURCE1} ./
Sources to use ============== Mirek is right, you can use the release tarball from GitHub. I had the impression that there is a strong requirement for Python packages to use the PyPI sources, but re-reading the relevant section from the Packaging Guidelines, it seems that the archive containing the tests (and license and docs) should be preferred: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_files_from_pypi Pinned version of virtualenv ============================ Going through the Git history it seems that the version of virtualenv was pinned when the upgrade from v16 to v20 happened, and was done to work around some braking API changes: https://github.com/JetBrains/teamcity-messages/commit/79f9429a61b8d644d8939b8c3395bc154ae2e17a#diff-60f61ab7a8d1910d86d9fda2261620314edcae5894d5aaa236b821c7256badd7 This was later bumped to the latest version in the v20 series, but there is no explanation provided neither in the commit message, nor in the PR discussion why continuing to pin the version was deemed to be necessary: https://github.com/JetBrains/teamcity-messages/pull/247 https://github.com/JetBrains/teamcity-messages/commit/71795a80ca407d34ac1973c1faf4c600ff04f74b So I assume it was a change done mechanically :) I think you should try opening a PR with upstream, removing that pinning and see what their feedback is. Meanwhile, you can carry that change as a patch downstream, and mention the upstream PR in the comment for the patch. Running only the unit tests =========================== I find using 'python setup.py test' to run the tests a little bit archaic, but it's referenced in more than just 'tox.ini', so modernizing this would be a bigger work in upstream. However, it seems that there is mechanism which allows passing some user arguments to 'tox', which could be used to limit the tests to be executed only to the unit tests. Usage examples can be found in the commit message which introduced this: https://github.com/JetBrains/teamcity-messages/commit/ca073bbf289ef4170188025665a488339aab5273 Based on the documentation of the '%tox' macro (https://src.fedoraproject.org/rpms/pyproject-rpm-macros/blob/rawhide/f/README.md) the following should work: %tox -- -- -a tests/unit-tests I recommend making a comment above this line, to tell that the doubel '--' *is intentional* and include a link to the pyproject macros documentation page. Note, that the '%tox' macro uses only the current Python environment to run 'tox', so there is no need to remove old envs from 'tox.ini' (that was a wrong advise from my side). I did not try the above. If it doesn't work, please let me know. If it works: would it be possible to remove the 'with tests' conditional, and let the tests run every build? Maybe this would require some manual 'BuildRequires'? Other ===== * Change '%pyproject_buildrequires -t' to '%pyproject_buildrequires -r', there are no dependencies defined in 'tox'. * Even if you don't use %pyproject_check_import anymore I would suggest to keep %pyproject_wheel and %pyproject_install. They are good macros, nevertheless :) * Note, that although for you Copr builds you need to bump the 'Release' tag, for the purpose of this review, that should stay '1', as this is going to be the first release in Fedora for version 1.30.
github sources ============== Following Mirek link I was able to build the right github URL, now nor rpmlint or rpmbuild are complaining anymore. Since the pypi archive does not only lacks tox.ini but also all the tests source directory, I will stay with the github archive. %pytest instead of %tox ======================= Instead of using tox I am just calling pytest with the unit tests. In this way I do not need the patches anymore. Since I am running just the unit tests I do not need all those BuildRequires (django, flake ecc.) and I removed them with the conditional statement too. I replaced Suggestions with Enhances as weak references to all those "requires" (django, flake, ecc.) %pyproject* =========== I went back using %pyproject_wheel and %pyproject_install, and %pyproject_buildrequires -r again. release ======= I put the release back to 1. I thought that I can simply remove the old builds inside COPR to be sure it will serve me the right one. New SPEC: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03258026-python-teamcity-messages/python-teamcity-messages.spec New sRPM: https://download.copr.fedorainfracloud.org/results/mmassari/teamcity-messages/fedora-rawhide-x86_64/03258026-python-teamcity-messages/python-teamcity-messages-1.30-1.fc36.src.rpm COPR build: https://copr.fedorainfracloud.org/coprs/mmassari/teamcity-messages/build/3258026/ KOJI build: https://koji.fedoraproject.org/koji/taskinfo?taskID=82059116 =============================================================================== rpmlint session starts =============================================================================== rpmlint: 2.2.0 configuration: /usr/lib/python3.10/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/licenses.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 1 ================================================ 0 packages and 1 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.5 s ================================================
Thank you, Maja! From my POV, this looks awesome and is ready to be accepted.
Wow, I have learned about %pycached - this was actually new to me :) LGTM - APPROVED Thank you Hunor and Jiri for the hardest part of the review. I just added Maja to the packager group.
FEDORA-2022-4e1ee4cb9e has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2022-4e1ee4cb9e
FEDORA-2022-4e1ee4cb9e has been pushed to the Fedora 37 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-fb6ff445cf has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-fb6ff445cf
FEDORA-2022-fb6ff445cf has been pushed to the Fedora 36 stable repository. If problem still persists, please make note of it in this bug report.