Fedora Account System
Red Hat Associate
Red Hat Customer
Spec URL: https://hiwkby.fedorapeople.org/python-kafka.spec SRPM URL: https://hiwkby.fedorapeople.org/python-kafka-2.0.2-1.fc36.src.rpm Description: This module provides low-level protocol support for Apache Kafka as well as high-level consumer and producer classes. Request batching is supported by the protocol as well as broker-aware request routing. Gzip and Snappy compression is also supported for message sets. Fedora Account System Username: hiwkby
Hi, I'm no sure I will have time to make a full review but here are few points that caught my attention: * Missing LICENSE file * You are using sources from PyPI. Do you have a reason to use those instead of official ones from Github ? More information about this topic here: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_source_files_from_pypi * I can see that tests are not performed according to guidelines. This is a one reason why building from PyPI is not always a great idea (missing tox files). More information here: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_running_tests * From changelog I can see that you un-retired this package, it would be good idea to update macros in spec file according to guidelines https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#_deprecated_macros Esthetic issues (aka rpmlint): It would be nice to fix those or add exceptions with a comment if necessary. * executable-script /usr/lib/python3.10/site-packages/kafka/record/_crc32c.py 644 /usr/bin/env python * python3-kafka.noarch: E: description-line-too-long This module provides low-level protocol support for Apache Kafka as well as high-level * python3-kafka.noarch: E: description-line-too-long consumer and producer classes. Request batching is supported by the protocol as well as * python3-kafka.noarch: E: description-line-too-long broker-aware
Hello mkulik, Thanks you very much for your review! and sorry for my late response. I will fix the issues today.
Hello mkulik, I have fixed the issues! It would be greatly appreciated if you could check it again. Spec URL: https://hiwkby.fedorapeople.org/python-kafka.spec SRPM URL: https://hiwkby.fedorapeople.org/python-kafka-2.0.2-2.fc36.src.rpm Koji scratch build(success!) https://koji.fedoraproject.org/koji/taskinfo?taskID=76217702 Thanks in advance, Hirotaka
No problem, here are few things that should be addressed after deeper look at package. * You got one typo in changelog: "- Wed Sep 24 2021 Hirotaka Wakabayashi (bogus date)" * Package provides some extra_requires that are optional. Maybe it's worth to check if (ZSTD, LZ4, snappy..) are correctly added, more info: https://docs.fedoraproject.org/en-US/packaging-guidelines/Python/#Extras I'm not sure if all extras should be included (testing usage), maybe none of them should be included. Investigate, fix or just make a comment regarding this. * I would generate additional docs (maybe pdf ?). Upstream uses sphinx so it should be easy. What also caught my eye is that you are using .diff for patch files. This is probably fine and correct but usually in other packages: python and not python ones .patch extensions is used. This is probably matter of preference so it's fine. Overall, good job. I will finish review after above issues are fixed/addressed.
Hello mkulik, Thanks you a lot for your review! I will fix the issues today!
Hello mkulik, I have fixed the issues! Please check it. https://hiwkby.fedorapeople.org/python-kafka.spec https://hiwkby.fedorapeople.org/python-kafka-2.0.2-3.fc36.src.rpm Koji(success!): https://koji.fedoraproject.org/koji/taskinfo?taskID=76359092 Thanks in advance, Hirotaka
Hi, You can continue with next steps. There is one more issue that I pointed out in 'Review comments' section that you should fix before. I finished reviewing this package. Good job :). Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - 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. Note: License file license.rst.txt is not marked as %license See: https://docs.fedoraproject.org/en-US/packaging- guidelines/LicensingGuidelines/#_license_text - Package does not use a name that already exists. Note: A package with this name already exists. Please check https://src.fedoraproject.org/rpms/python-kafka See: https://docs.fedoraproject.org/en-US/packaging- guidelines/Naming/#_conflicting_package_names Reviewer comments: ======= - One more thing, .js libraries in -doc package are provided with mixed licenses. I will approve it because licenses are present but you should fix this issues for -doc subpackage before continuing. (%license macro and License: in .spec file for this subpackage) Tips: - We can specify multiple packages with pyproject_extras_subpkg. Example: %pyproject_extras_subpkg -n python3-requests security socks ===== 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. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "*No copyright* Apache License 2.0", "*No copyright* Apache License", "Apache License 2.0", "MIT License", "*No copyright* zlib License". 188 files have unknown license. [x]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [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]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [x]: Package is not known to require an ExcludeArch tag. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 71680 bytes in 2 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]: 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]: 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 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: [?]: 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]: Patches link to upstream bugs/comments/lists or are otherwise justified. [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. Source checksums ---------------- https://github.com/dpkp/kafka-python/archive/refs/tags/2.0.2.tar.gz : CHECKSUM(SHA256) this package : 5dcf87c559e7aee4f18d621a02e247db3e3552ee4589ca611d51eef87b37efed CHECKSUM(SHA256) upstream package : 5dcf87c559e7aee4f18d621a02e247db3e3552ee4589ca611d51eef87b37efed Requires -------- python3-kafka (rpmlib, GLIBC filtered): /usr/bin/python3 python(abi) python-kafka-doc (rpmlib, GLIBC filtered): python3-kafka+zstd (rpmlib, GLIBC filtered): python(abi) python3-kafka python3.10dist(zstandard) Provides -------- python3-kafka: python-kafka python3-kafka python3.10-kafka python3.10dist(kafka-python) python3dist(kafka-python) python-kafka-doc: python-kafka-doc python3-kafka+zstd: python-kafka+zstd python3-kafka+zstd python3.10-kafka+zstd python3.10dist(kafka-python[zstd]) python3dist(kafka-python[zstd])
Hi mkulik, Thank you very much for your great review! I learned a lot! I will fix the license issue of doc subpackage today. Best Regards, Hirotaka
Hi mkulik, I have fixed the issue! I will request unretirement. https://hiwkby.fedorapeople.org/python-kafka.spec https://hiwkby.fedorapeople.org/python-kafka-2.0.2-4.fc36.src.rpm Hirotaka
I fixed an rpmlint error. Spec URL: https://hiwkby.fedorapeople.org/python-kafka.spec SRPM URL: https://hiwkby.fedorapeople.org/python-kafka-2.0.2-5.fc36.src.rpm Hirotaka
Hi hiwkby, You can close this ticket according to guidelines: https://docs.fedoraproject.org/en-US/package-maintainers/Package_Review_Process/#review_process