Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.1-1.fc43.src.rpm Description: A fully YAML 1.2 compliant YAML parser. Fedora Account System Username: lecris
Copr build: https://copr.fedorainfracloud.org/coprs/build/8888309 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/08888309-rust-yaml-rust2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.1-1.fc43.src.rpm Moved the patches to `rust2rpm -p`
Created attachment 2084769 [details] The .spec file difference from Copr build 8888309 to 8899929
Copr build: https://copr.fedorainfracloud.org/coprs/build/8899929 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/08899929-rust-yaml-rust2/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.1-1.fc43.src.rpm Relax hashlink requirement Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=132183326
Copr build: https://copr.fedorainfracloud.org/coprs/build/8987843 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/08987843-rust-yaml-rust2/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.
This project is ... weird? 1. The license files are in a hidden folder. They are identical (as far as I can tell) except the copyright attribution line. The Apache-2.0 license does *not* have a "Copyright (xxxx) Foo Bar" in the canonical copy, so it is strange to add it "downstream". The MIT license could just have "Copyright (xxxx-yyyy) Foo, (yyyy-zzzz) Bar" instead of containing the license text twice. Do you think it would be possible to make some suggestions to upstream (i.e. drop nonstandard Copyright lines from the Apache license text and move *one* copy to /LICENSE-APACHE, and merge the Copyright lines from the MIT license texts and move it to /LICENSE-MIT)? 2. There's a bunch of files included in published crates that are not interesting to downstream users. It would be great if they were excluded upstream, but excluding them downstream for now would be fine: - /documents/ - /tests/yaml-test-suite/ - /.cargo/ - /.github/ - /appveyor.yml - /garden.yaml - /justfile 3. The crate seems to include a vendored copy of the "yaml-test-suite" project, which is a bunch of Perl and Bash scripts (which also result in dependencies for /usr/bin/perl and /usr/bin/bash to get generated for the -devel package), a bunch of test input data, and it is also covered by a different license (MIT only). It looks like the /tests/yaml-test-suite/ folder should just not be included in published crates.
Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.1-1.fc43.src.rpm I've raised the license simplification issue upstream, let's see if they are up for it. Other than that I've removed the unnecessary files. I'll see how they want to handle the MIT only files Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=132732275
Created attachment 2089844 [details] The .spec file difference from Copr build 8987843 to 9040479
Copr build: https://copr.fedorainfracloud.org/coprs/build/9040479 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/09040479-rust-yaml-rust2/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.
Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.2-1.fc43.src.rpm Update to 0.10.2. Licenses are merged and the metadata reflects the license of the yaml-test-suite. The licenses are still in a `.license`, but at this point, I'll take it. Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=133018084
Created attachment 2090921 [details] The .spec file difference from Copr build 9040479 to 9066862
Copr build: https://copr.fedorainfracloud.org/coprs/build/9066862 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/09066862-rust-yaml-rust2/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.
Thanks, looks pretty good now, two issues left: 0. The upstream license expression is wrong, it's "MIT OR (MIT AND Apache-2.0)" whereas it should be "MIT AND (MIT OR Apache-2.0)". 1. Since you exclude yaml-test-suite from the built packages, the license tag should be just "MIT OR Apache-2.0" since no MIT-only contents end up in the built package. This needs to be patched in Cargo.toml too so the "wrong" expression doesn't affect the %cargo_license{,_summary} macros.
> 0. The upstream license expression is wrong, it's "MIT OR (MIT AND Apache-2.0)" whereas it should be "MIT AND (MIT OR Apache-2.0)". Alternatively, the upstream project could just exclude the yaml-test-suite stuff from published crates, that would eliminate the license expression issue entirely ...
(In reply to Fabio Valentini from comment #14) > 0. The upstream license expression is wrong, it's "MIT OR (MIT AND > Apache-2.0)" whereas it should be "MIT AND (MIT OR Apache-2.0)". My bad, I was the one who suggested the former. Isn't it equivalent in some sense? But yeah, it would affect the concatenation I suppose. > 1. Since you exclude yaml-test-suite from the built packages, the license > tag should be just "MIT OR Apache-2.0" since no MIT-only contents end up in > the built package. This needs to be patched in Cargo.toml too so the "wrong" > expression doesn't affect the %cargo_license{,_summary} macros. Well, this is the confusion, is `license` the license of the crate or the artifacts? If it's the former, then isn't theirs as `MIT AND (MIT OR Apache-2.0)` correct, and in our case different because we don't propagate? > Alternatively, the upstream project could just exclude the yaml-test-suite stuff from published crates, that would eliminate the license expression issue entirely It would limit our ability to run those tests if they fix the `libtest-mimic` dependency issue. It does seem like a good test suite to cover, but it might be sufficient to be covered downstream only? I am not sure which way I am leaning on this one.
> Isn't it equivalent in some sense? No, "A AND (A OR B)" and "A OR (A AND B)" are strictly different. The former is correct, the latter is wrong :D > Well, this is the confusion, is `license` the license of the crate or the artifacts? If it's the former, then isn't theirs as `MIT AND (MIT OR Apache-2.0)` correct, and in our case different because we don't propagate? Yes, it's possible that applying different standards for *what* needs to be represented in the license expression lead to different license expressions needing to be in place upstream and downstream. > It would limit our ability to run those tests if they fix the `libtest-mimic` dependency issue. True, but this seems to be a small issue (just skipping one test file?).
Spec URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2.spec SRPM URL: https://lecris.fedorapeople.org/reviews/rust-yaml-rust2/rust-yaml-rust2-0.10.3-1.fc43.src.rpm Update to 0.10.3 which fixed the license and bundling. I can actually make the yaml-test-suite work, but I will tackle that later after conference season is over. Any objections for adding a Source2 with the git archive of yaml-test-suite later on? Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=133869203
Created attachment 2093776 [details] The .spec file difference from Copr build 9066862 to 9157228
Copr build: https://copr.fedorainfracloud.org/coprs/build/9157228 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2358954-rust-yaml-rust2/fedora-rawhide-x86_64/09157228-rust-yaml-rust2/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.
Thank you for your work, this looks good now! Some last comments and one non-blocking suggestion included below, but the package is APPROVED. --- > Update to 0.10.3 which fixed the license and bundling. I can actually make the yaml-test-suite work, but I will tackle that later after conference season is over. Any objections for adding a Source2 with the git archive of yaml-test-suite later on? No, I have no objections to that, it sounds like a good solution all around. You'll even only need to unpack those sources in %check so there's no way they could even get installed as part of the built packages. --- Note that the statement in the LICENSE file is now inaccurate (it's only "MIT OR Apache-2.0" now, not "either only MIT or MIT AND Apache-2.0"), but at least the package is correct now and we will ignore that statement in the LICENSE file because we know it's wrong ;) --- Last suggestion (but this is not blocking the review): Shipping "hidden" files / folders is usually frowned upon in packaging. I would recommend that you replace > install -Dp .licenses/* -t %{buildroot}%{crate_instdir}/.licenses after %cargo_install with something like > mv .licenses/* ./ && rmdir .licenses *before* %cargo_install, and list the files as just %license "%{crate_instdir}/LICENSE-Apache" and "%{crate_instdir}/LICENSE-MIT". You can even do both in rust2rpm.toml by setting `package.license-files = ["LICENSE-Apache", "LICENSE-MIT"]` and putting the pre-%cargo_install steps into the `scripts.install.pre` setting. --- Package was generated with rust2rpm, simplifying the review. ✅ package contains only permissible content ✅ package builds and installs without errors on rawhide ✅ test suite is run and all unit tests pass ✅ latest version of the crate is packaged ✅ license matches upstream specification and is acceptable for Fedora ✅ license files are included with %license in %files ✅ package complies with Rust Packaging Guidelines Package APPROVED. === Recommended post-import rust-sig tasks: - set up package on release-monitoring.org: project: $crate homepage: https://crates.io/crates/$crate backend: crates.io version scheme: semantic version filter (*NOT* pre-release filter): alpha;beta;rc;pre distro: Fedora Package: rust-$crate - set bugzilla assignee overrides to @rust-sig (optional)