Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/alebastr/rust-tree-sitter-cli/fedora-rawhide-x86_64/02999989-rust-tree-sitter-cli/rust-tree-sitter-cli-0.20.1-2.fc36.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/alebastr/rust-tree-sitter-cli/build/2999989/ Description: CLI tool for developing, testing, and using Tree-sitter parsers Fedora Account System Username: alebastr Note: generated with `rust2rpm -a`; added upstream license file as required by the MIT license; calculated the resulting binary license from all the crates in the buildroot.
Initial comments: - You modified the "License" tag of the source package (which is still "only MIT") instead of that of the built package that contains the statically linked binary. You can take a look here for an example how to specify binary package license correctly: https://src.fedoraproject.org/rpms/rust-fedora-update-feedback/blob/rawhide/f/rust-fedora-update-feedback.spec The command to run in mock shell after a build "--without check" and `mock install dnf-utils` to generate the list of packages+licenses is `dnf repoquery --cacheonly "rust-*-devel" --installed --qf "# %{LICENSE}: %{source_name} %{version}"` - You don't need to modify the file name of the license if you're taking it verbatim from upstream.
(In reply to Fabio Valentini from comment #1) > Initial comments: > > - You modified the "License" tag of the source package (which is still "only > MIT") instead of that of the built package that contains the statically > linked binary. You can take a look here for an example how to specify binary > package license correctly: > https://src.fedoraproject.org/rpms/rust-fedora-update-feedback/blob/rawhide/ > f/rust-fedora-update-feedback.spec Right. Thanks for catching! It's embarrassing, because I knew this and even confirmed with the packaging guidelines... but forgot to apply :) > > The command to run in mock shell after a build "--without check" and `mock > install dnf-utils` to generate the list of packages+licenses is > `dnf repoquery --cacheonly "rust-*-devel" --installed --qf "# %{LICENSE}: > %{source_name} %{version}"` I actually used the script from the rust2rpm repo. I prefer a shorter list of unique license tags. Will recheck if the result is matching, just in case. > - You don't need to modify the file name of the license if you're taking it > verbatim from upstream. The license file is small enough to prefer keeping it in git. In this case LICENSE in the root of a src.fp.o package repository could be misinterpreted as a license for the package spec, and the point of renaming is to avoid the ambiguity. Thanks for the review!
> I actually used the script from the rust2rpm repo. I prefer a shorter list of unique license tags. > Will recheck if the result is matching, just in case. The resulting License tag should be the same. However, I've gotten criticism about using just the "short list" version of the licenses as a comment, since it's actually pretty useless (no information about which bundled dependency is included or under which license it is), which is why I started including the more detailed list in all packages I update. > The license file is small enough to prefer keeping it in git. In this case LICENSE in the root of a src.fp.o package repository could be misinterpreted as a license for the package spec, and the point of renaming is to avoid the ambiguity. That is true. If you want to commit it into git instead of adding it as a Source, that makes sense. === For the final review, I'd like to wait until the support for passing CFLAGS etc. is merged into the rust macros. Because I don't like the idea of having parsers for potentially untrusted input written in C being compiled without security hardening flags :)
Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/alebastr/rust-tree-sitter-cli/fedora-rawhide-x86_64/03031943-rust-tree-sitter-cli/rust-tree-sitter-cli-0.20.1-4.fc36.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/build/3031943 Changes: - applied license to the correct subpackage. I have an expanded list of licenses[1] and will add that before import. Although I'm considering to keep it as a separate file next to the spec and refer to the file in a comment. - enabled tests. After addressing some type mismatch issues on arm I decided that it's better to have the tests :) Test dependencies are in a separate archive, with the script to generate it attached. Licenses checked; all unnecessary files are excluded and nothing from that archive gets to the binary packages. I believe that's all I wanted to do with the packages, so let's wait for the next rust-packaging macros release [1]: https://alebastr.fedorapeople.org/review/tree-sitter-cli/licenses.txt
Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/alebastr/rust-tree-sitter-cli/fedora-rawhide-x86_64/03540803-rust-tree-sitter/rust-tree-sitter-0.20.2-1.fc37.src.rpm Git repo with all the extra files: https://git.alebastr.su/rpms/rust-tree-sitter-cli As I noted in bz2028895, anything > 0.20.2 is not compatible with tree-sitter-0.20.1 and fails the test suite. Tests are failing on s390x (`memory allocation of 412316860416 bytes failed` -- I suspect there are issues with BE architectures) so I'm going to %ifarch the tests and look into that when I have enough time.
> As I noted in bz2028895, anything > 0.20.2 is not compatible with tree-sitter-0.20.1 and fails the test suite. If you need 0.20.2 for compatibility with the existing tree-sitter library in Fedora, then that's a good enough reason for me. If you want to investigate the endianness issues, it *might* actually be enough to look through the code for uses of "to_le_bytes" or "from_le_bytes", and change those to "to_ne_bytes" and "from_ne_bytes" to fix the issue.
Looks like you posted the link to the wrong SRPM above. The SRPM link points to rust-tree-sitter SRPM but not a rust-tree-sitter-cli SRPM.
Right... I guess I was half-asleep when I updated the bug. Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://download.copr.fedorainfracloud.org/results/alebastr/rust-tree-sitter-cli/fedora-rawhide-x86_64/03542539-rust-tree-sitter-cli/rust-tree-sitter-cli-0.20.2-2.fc37.src.rpm
The link for that SRPM file now returns HTTP 404 :)
(In reply to Fabio Valentini from comment #9) > The link for that SRPM file now returns HTTP 404 :) Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli-0.20.2-1.fc35.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/build/3667603 (ignore i386 - it failed to fetch repodata from koji)
Looks good to me, with four minor exceptions: 1) Patch0: test-use-platform-independent-c_char-instead-of-i8.patch 2) # tests are failing on s390x Please submit the patch for 1) upstream, and report an upstream issue for 2). I also wonder if you could replace this: # Some tests require the C library development files # Not a pkgconfig dep because of tree-sitter/tree-sitter#1158 BuildRequires: libtree-sitter-devel >= 0.20 With setting INCLUDEDIR or something like that to the path of the tree-sitter sources that are bundled with rust-tree-sitter anyway. 3) # See LICENSE.dependencies for a full list of buildroot crates and licenses This file seems to be missing from the SRPM. It's a nice idea to put this into separate file, though. I might steal that for some of my packages. You might want to include the "executive summary" in the .spec file though, so it at least contains a list of the individual licenses, if not the complete breakdown, i.e. what's printed by `for i in $(rpm -qa | grep "rust-.*-devel"); do rpm -q $i --qf "%{LICENSE}\n"; done | sort | uniq`. 4) Please exclude the npm / NodeJS files (npm/, emscripten-version) from getting installed with the -devel package, and investigate whether you need the "vendor/xterm-colors.json" file. You can either %exclude them in the %files devel list, or add an "exclude = []" snippet to Cargo.toml.
Spec URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli.spec SRPM URL: https://alebastr.fedorapeople.org/review/tree-sitter-cli/rust-tree-sitter-cli-0.20.6-1.fc36.src.rpm Copr build: https://copr.fedorainfracloud.org/coprs/build/3938868 The patch was already sent to upstream in the bindgen PR, but I just made another PR for all accumulated architecture support fixes[1] and an issue[2] for the only remaining failure. > I also wonder if you could replace this: > With setting INCLUDEDIR or something like that to the path of the tree-sitter sources that are bundled with rust-tree-sitter anyway. Yup, good idea. Nothing there needs the shared library from libtree-sitter anyways and it's better to match the headers used for tests with the bundled library source. Replaced with an extremely evil symlink to %{cargo_registry}/tree-sitter-... and left an explanation with source code link. It really has to be done that way :( I'm open to ideas on replacing a lovely reference to '../lib'[3] with a path to another crate's source. I had no luck searching for a code snippet or library :) > This file seems to be missing from the SRPM. I don't think it's worth adding the file to the SRPM as it doesn't participate in the build and I just added a licensing summary. I'm going to keep the file in dist-git though. Does that sound fine? > Please exclude the npm / NodeJS files (npm/, emscripten-version) from getting installed with the -devel package, and investigate whether you need the "vendor/xterm-colors.json" file. emscripten-version[4] and vendors/xterm-colors.json[5] both are necessary for building the crate. npm/ is unused though, so I'm removing it now. Other changes: - updated to 0.20.6 as we're no longer care about the libtree-sitter version (well, mostly...) - added %ifarch conditionals for a test that requires nodejs [1]: https://github.com/tree-sitter/tree-sitter/pull/1692 [2]: https://github.com/tree-sitter/tree-sitter/issues/1693 [3]: https://github.com/tree-sitter/tree-sitter/blob/ccd6bf554d922596ce905730d98a77af368bba5c/cli/src/tests/helpers/dirs.rs#L4 [4]: https://github.com/tree-sitter/tree-sitter/blob/ccd6bf554d922596ce905730d98a77af368bba5c/cli/build.rs#L19 [5]: https://github.com/tree-sitter/tree-sitter/blob/ccd6bf554d922596ce905730d98a77af368bba5c/cli/src/highlight.rs#L44
> Replaced with an extremely evil symlink to %{cargo_registry}/tree-sitter-... and left an explanation with source code link. It really has to be done that way :( > > I'm open to ideas on replacing a lovely reference to '../lib'[3] with a path to another crate's source. I had no luck searching for a code snippet or library :) Looks good to me. Since those headers are apparently only required for running the test suite, it probably doesn't make sense to tie the version of this crate to tree-sitter-devel version just for that. > I don't think it's worth adding the file to the SRPM as it doesn't participate in the build and I just added a licensing summary. I'm going to keep the file in dist-git though. Does that sound fine? If that's what you want to do, that's fine with me. For some recent packages I updated, I added the file as a "Source" file that I committed it to dist-git, and added it to the binary package with "%license LICENSE.dependencies", but you don't necessarily have to do that. example: https://src.fedoraproject.org/rpms/rust-sequoia-sqv/c/63355b63efe8ed82d1289f11e54ef0fb57b15d72?branch=rawhide > emscripten-version[4] and vendors/xterm-colors.json[5] both are necessary for building the crate. npm/ is unused though, so I'm removing it now. LGTM. > - added %ifarch conditionals for a test that requires nodejs Uh, okay. If you want to keep this complexity, that's up to you. I'd probably just disable that test unconditionally, so I wouldn't have to care about which architectures are supported by NodeJS. === Other than that, the package already looked good, so here's the final review (sorry for the delay): Package was generated with rust2rpm, simplifying the review. - package builds and installs without errors on rawhide (with latest koji repos) - test suite is run and all unit tests pass (or are reported to upstream) - latest version of the crate is packaged - license matches upstream specification (MIT) and is acceptable for Fedora - license file is included with %license in %files - package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import rust-sig tasks: - add @rust-sig with "commit" access as package co-maintainer - set bugzilla assignee overrides to @rust-sig (optional) - set up package on release-monitoring.org: project: $crate homepage: https://crates.io/crates/$crate backend: crates.io version scheme: semantic version filter: alpha;beta;rc;pre distro: Fedora Package: rust-$crate - track package in koschei for all built branches
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-tree-sitter-cli
FEDORA-2022-fc2f24385e has been submitted as an update to Fedora 36. https://bodhi.fedoraproject.org/updates/FEDORA-2022-fc2f24385e
FEDORA-2022-50a7dedd38 has been submitted as an update to Fedora 35. https://bodhi.fedoraproject.org/updates/FEDORA-2022-50a7dedd38
FEDORA-2022-50a7dedd38 has been pushed to the Fedora 35 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --advisory=FEDORA-2022-50a7dedd38 \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-50a7dedd38 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-fc2f24385e 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 --advisory=FEDORA-2022-fc2f24385e \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2022-fc2f24385e See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2022-50a7dedd38 has been pushed to the Fedora 35 stable repository. If problem still persists, please make note of it in this bug report.
FEDORA-2022-fc2f24385e has been pushed to the Fedora 36 stable repository. If problem still persists, please make note of it in this bug report.