Spec URL: https://music.fedorapeople.org/cramjam-cli.spec SRPM URL: https://music.fedorapeople.org/cramjam-cli-0.1.1-1.fc39.src.rpm Description: Simple CLI to a variety of compression algorithms. Fedora Account System Username: music
Copr build: https://copr.fedorainfracloud.org/coprs/build/7020743 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2264463-cramjam-cli/fedora-rawhide-x86_64/07020743-cramjam-cli/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.
[fedora-review-service-build] Re-uploaded (same URLs) with a more straightforward approach to the %files list, and with the Python metadata packaged in a subpackage so that the base package doesn’t have to depend on Python. I could omit the Python metadata altogether, but I feel like I should retain it in the spirit of staying close to upstream, especially since PyPI is the primary distribution channel. CC’ing a few people who might be interested in the existence of a pure-Rust command-line tool, built using Python tooling (pyproject.toml with maturin) and distributed via PyPI (https://pypi.org/project/cramjam-cli/), apparently to make it convenient to install. Suggestions welcome.
Created attachment 2017032 [details] The .spec file difference from Copr build 7020743 to 7020814
Copr build: https://copr.fedorainfracloud.org/coprs/build/7020814 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2264463-cramjam-cli/fedora-rawhide-x86_64/07020814-cramjam-cli/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.
Hm ... it looks like the Python subpackage only contains metadata but nothing else? Is there actually any reason to build this as a Python package then? "Convenience" for installing it via pip isn't really an argument for us, and it just makes the package more complicated.
[fedora-review-service-build]
Apparently, the fedora-review.txt created by the review service is empty
Hmm, I suppose we could simply do %pyproject_install rm -rv %{buildroot}%{python3_sitearch}/cramjam_cli-%{version}.dist-info Or, it turns out it is possible to build and install this with %cargo_build/%cargo_install instead of %pyproject_wheel/%pyproject_install, which would prevent the Python metadata from being generated in the first place. I still need the Python BuildRequires to run the tests, and I still need to package from the PyPI sdist because cramjam-cli is not published on crates.io and the commits corresponding its releases are not currently explicitly tagged in the GitHub repository. Still, maybe one of the above approaches is the way to go. Anything that currently tries to depend on cramjam-cli as a Python package and then call the command-line tool via subprocess or similar would need patching, but that’s already the status quo for some other other attempts to hackily deliver executables via PyPI (https://pypi.org/project/cmake/, https://pypi.org/project/ninja/). Why, and whether, anything would do that when https://pypi.org/project/cramjam/ is available is another question. I’ll let this sit as-is for the moment and possibly revise it a little later.
Copr build: https://copr.fedorainfracloud.org/coprs/build/7020890 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2264463-cramjam-cli/fedora-rawhide-x86_64/07020890-cramjam-cli/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.
(In reply to Fabio Valentini from comment #5) > Hm ... it looks like the Python subpackage only contains metadata but > nothing else? > > Is there actually any reason to build this as a Python package then? > "Convenience" for installing it via pip isn't really an argument for us, and > it just makes the package more complicated. After some consideration, I decided to take this advice. We can always start generating and packaging the metadata if someone makes a really good argument for it in the future. New Spec URL: https://music.fedorapeople.org/20240216/cramjam-cli.spec New SRPM URL: https://music.fedorapeople.org/20240216/cramjam-cli-0.1.1-1.fc39.src.rpm
> ...if someone makes a really good argument for it in the future. I suppose this would matter when some Python package generates a dependency on python3dist(cramjam-cli).
> # needed to package this metadata, we would probably wnat to do it via a typo "wnat"
(In reply to Miro Hrončok from comment #12) > > # needed to package this metadata, we would probably wnat to do it via a > > typo "wnat" Thanks; fixed and re-uploaded with the same URLs.
(In reply to Miro Hrončok from comment #11) > > ...if someone makes a really good argument for it in the future. > > I suppose this would matter when some Python package generates a dependency > on python3dist(cramjam-cli). Exactly. Will that ever happen? No idea. This kind of thing already happens with python3dist(cmake) and python3dist(ninja), corresponding to https://pypi.org/project/cmake/ and https://pypi.org/project/ninja/. When Python packages generate dependencies on those, they have to be patched out and replaced with manual BuildRequires on cmake and ninja-build, respectively. Admittedly, the the PyPI projects for CMake and Ninja are more of a “hack” rather than the primary upstream distribution channel, but they *are* published “officially” by the respective upstreams. Alternatively, maybe we *should* be trying to package something that satisfies python3dist(cmake) and python3dist(ninja).
I preferred the previous approach where you included the Python metadata. PyPI is the primary distribution channel, and I don't think including the Python package adds _too_ much extra complication. It seems strange to me to use the sdist but not build a Python package from it.
(In reply to Maxwell G from comment #15) > I preferred the previous approach where you included the Python metadata. > PyPI is the primary distribution channel, and I don't think including the > Python package adds _too_ much extra complication. It seems strange to me to > use the sdist but not build a Python package from it. Do you like the original approach of having a separate python3-cramjam-cli package, with a fully-versioned dependency on the base package, to carry the metadata and the Python interpreter dependency? Miro and Fabio, what do you think? I’m kind of taking a poll here. I think either approach (package metadata or don’t) is defensible, and neither approach is going to burden me too much.
(In reply to Ben Beasley from comment #16) > (In reply to Maxwell G from comment #15) > > I preferred the previous approach where you included the Python metadata. > > PyPI is the primary distribution channel, and I don't think including the > > Python package adds _too_ much extra complication. It seems strange to me to > > use the sdist but not build a Python package from it. > > Do you like the original approach of having a separate python3-cramjam-cli > package, with a fully-versioned dependency on the base package, to carry the > metadata and the Python interpreter dependency? Yes, exactly.
My preferences, from best (1) to worst (2): 1) don't build/install a Python wheel, use Rust macros 2) build/install a Python wheel, but delete the dist-info 3) keep the dist-info in a subpackage 4) treat this as a Python application package, keep the dist-info in the "main" package If you do 1 or 2 and somebody asks for the metadata, give them 3 or 4.
OK, it sounds like two are in favor of the latest submission using approach (1), and one prefers the original submission using (3). I appreciate all of the input. I’m going to stick with the current approach (1) for now, with the understanding that there are good arguments for both approaches, and that the spec-file comments document how to implement (3) so that it can be added promptly if it’s ever needed. https://www.wheelodex.org/projects/cramjam-cli/rdepends/ suggests that nothing on PyPI has a Python dependency on cramjam-cli so far.
Created attachment 2017283 [details] The .spec file difference from Copr build 7020890 to 7027753
Copr build: https://copr.fedorainfracloud.org/coprs/build/7027753 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2264463-cramjam-cli/fedora-rawhide-x86_64/07027753-cramjam-cli/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.
It occurred to me that when someone packages the new “uv,”[1][2][3], the situation will be very similar. It’s an executable written entirely in Rust, without a strict dependency on a Python interpreter, but it’s distributed primarily on PyPI. Only, since it’s intended as a drop-in replacement for pip, I expect that it will be necessary to package its Python metadata so it can provide python3dist(uv). [1] https://astral.sh/blog/uv [2] https://github.com/astral-sh/uv [3] https://pypi.org/project/uv
Package looks good to me. Full review below. Some notes: 0. Shipping Python metadata would be fine with me, reading up on previous comments. But it could always be added later if it turns out to be needed for something. 1. The no-strip patch is no longer necessary since you no longer use maturin for building. You could drop it - or keep it, if you think switching back to maturin + shipping Python metadata might happen in the future, and want to keep this "documented" until then. 2. You might want to add "ExcludeArch: i686" since this is a new leaf package, but that is your decision. 3. There is no manual page for the CLI application. I don't mind, but you told me you like them. It looks like cramjam-cli uses clap for constructing its CLI, so it should have "useful" help output that can be piped to help2man :) === Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed ===== 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. [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [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]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [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. [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 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]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [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). [?]: 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. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. [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]: 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]: Fully versioned dependency in subpackages if applicable. [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 debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: cramjam-cli-0.1.1-1.fc41.x86_64.rpm cramjam-cli-debuginfo-0.1.1-1.fc41.x86_64.rpm cramjam-cli-debugsource-0.1.1-1.fc41.x86_64.rpm cramjam-cli-0.1.1-1.fc41.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmprdmwu0ov')] checks: 32, packages: 4 cramjam-cli.x86_64: W: no-manual-page-for-binary cramjam-cli 4 packages and 0 specfiles checked; 0 errors, 1 warnings, 17 filtered, 0 badness; has taken 0.4 s Rpmlint (debuginfo) ------------------- Checking: cramjam-cli-debuginfo-0.1.1-1.fc41.x86_64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmp8z8km0p5')] checks: 32, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 5 filtered, 0 badness; has taken 0.1 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.5.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 32, packages: 3 cramjam-cli.x86_64: W: no-manual-page-for-binary cramjam-cli 3 packages and 0 specfiles checked; 0 errors, 1 warnings, 14 filtered, 0 badness; has taken 0.3 s Source checksums ---------------- https://files.pythonhosted.org/packages/source/c/cramjam_cli/cramjam_cli-0.1.1.tar.gz : CHECKSUM(SHA256) this package : 45d47a484f42d967b8ce2aa73c72fa70ab7d58bb11c12818c5631a646e0fe16d CHECKSUM(SHA256) upstream package : 45d47a484f42d967b8ce2aa73c72fa70ab7d58bb11c12818c5631a646e0fe16d Requires -------- cramjam-cli (rpmlib, GLIBC filtered): ld-linux-x86-64.so.2()(64bit) libbz2.so.1()(64bit) libc.so.6()(64bit) libgcc_s.so.1()(64bit) libgcc_s.so.1(GCC_3.0)(64bit) libgcc_s.so.1(GCC_3.3)(64bit) libgcc_s.so.1(GCC_4.2.0)(64bit) liblz4.so.1()(64bit) libm.so.6()(64bit) libzstd.so.1()(64bit) rtld(GNU_HASH) cramjam-cli-debuginfo (rpmlib, GLIBC filtered): cramjam-cli-debugsource (rpmlib, GLIBC filtered): Provides -------- cramjam-cli: cramjam-cli cramjam-cli(x86-64) cramjam-cli-debuginfo: cramjam-cli-debuginfo cramjam-cli-debuginfo(x86-64) debuginfo(build-id) cramjam-cli-debugsource: cramjam-cli-debugsource cramjam-cli-debugsource(x86-64)
(In reply to Ben Beasley from comment #22) > It occurred to me that when someone packages the new “uv,”[1][2][3], the > situation will be very similar. It’s an executable written entirely in Rust, > without a strict dependency on a Python interpreter, but it’s distributed > primarily on PyPI. Only, since it’s intended as a drop-in replacement for > pip, I expect that it will be necessary to package its Python metadata so it > can provide python3dist(uv). > > [1] https://astral.sh/blog/uv > [2] https://github.com/astral-sh/uv > [3] https://pypi.org/project/uv TIL. Are there plans to package this?
(In reply to Fabio Valentini from comment #24) > TIL. Are there plans to package this? No idea! But given it’s “by the ruff people,” I would expect it to catch on quickly enough in the broader Python community that packaging it becomes necessary.
(In reply to Fabio Valentini from comment #23) > Package looks good to me. Full review below. Some notes: > > 0. Shipping Python metadata would be fine with me, reading up on previous > comments. But it could always be added later if it turns out to be needed > for something. I think I’ll stay the course for now, but if this Rust/maturin/PyPI executable pattern becomes popular, I suspect we will need to settle on a default approach that does package the Python metadata. > 1. The no-strip patch is no longer necessary since you no longer use maturin > for building. You could drop it - or keep it, if you think switching back to > maturin + shipping Python metadata might happen in the future, and want to > keep this "documented" until then. Hmm, good point. I think I’ll keep the patch, but add a comment documenting that it doesn’t matter when building directly with cargo. > 2. You might want to add "ExcludeArch: i686" since this is a new leaf > package, but that is your decision. This is a good idea, and I’ll do it. > 3. There is no manual page for the CLI application. I don't mind, but you > told me you like them. It looks like cramjam-cli uses clap for constructing > its CLI, so it should have "useful" help output that can be piped to > help2man :) I do like having man pages, but only ones that are useful. Unfortunately, help2man generates a terrible man page in this case: - The list of subcommands is poorly formatted and jumbled inline with the description of the “help” subcommand - The example is tacked onto the end of the OPTIONS section instead of placed in an EXAMPLES section - The -l/--level option is documented only in the subcommand help output, e.g. cramjam-cli snappy --help. - It’s not clear from the generated output that all of the subcommands take exactly the same options. Since it doesn’t look like the CLI options are going to change much (particularly, it doesn’t look like there will be low-level algorithm-specific options), so the maintenance burden won’t be too high, I may in fact use the --help output to hand-write a man page that is properly formatted and actually useful.
The Pagure repository was created at https://src.fedoraproject.org/rpms/cramjam-cli
https://release-monitoring.org/project/345571/
FEDORA-2024-c8b6669250 (cramjam-cli-0.1.1-1.fc41) has been submitted as an update to Fedora 41. https://bodhi.fedoraproject.org/updates/FEDORA-2024-c8b6669250
FEDORA-2024-c8b6669250 (cramjam-cli-0.1.1-1.fc41) has been pushed to the Fedora 41 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-3d1c406c8f (cramjam-cli-0.1.1-1.fc40) has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2024-3d1c406c8f
FEDORA-2024-3d1c406c8f (cramjam-cli-0.1.1-1.fc40) has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-287d1573bc (cramjam-cli-0.1.1-1.fc39) has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2024-287d1573bc
FEDORA-2024-0324965cf4 (cramjam-cli-0.1.1-1.fc38) has been submitted as an update to Fedora 38. https://bodhi.fedoraproject.org/updates/FEDORA-2024-0324965cf4
FEDORA-2024-287d1573bc has been pushed to the Fedora 39 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-287d1573bc \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-287d1573bc See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-0324965cf4 has been pushed to the Fedora 38 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-0324965cf4 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-0324965cf4 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2024-287d1573bc (cramjam-cli-0.1.1-1.fc39) has been pushed to the Fedora 39 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2024-0324965cf4 (cramjam-cli-0.1.1-1.fc38) has been pushed to the Fedora 38 stable repository. If problem still persists, please make note of it in this bug report.