Bug 2362051

This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=131936217

Copr build:
https://copr.fedorainfracloud.org/coprs/build/8963696
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2362051-rust-sccache/fedora-rawhide-x86_64/08963696-rust-sccache/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.

Taking this review.

(Note that setting fedora-review? yourself is wrong, it just leaves the bug in an inconsistent state that probably makes it not show up on the dashboard here: https://fedoraproject.org/PackageReviewStatus/reviewable.html )

Ping?

Sorry for the delay, was busy with other things last week.

There's some issues I found (numbered for easier reference):

1) The FIXME items in the generated spec are not addressed - i.e. the LICENSE tag for the subpackage that contains the statically linked executable needs to be determined and filled in.

2) The patch for Cargo.toml doesn't look entirely correct, especially when looking at the -f flags passed to the %cargo_* macros too:

If you want to drop support for the server-side part (and the rouille dependency) - i.e. the "dist-server" and "rouille" features - don't add dummy feature flags for them, just drop them entirely. Having dummy feature flags makes it *look* like the crate supports these features, but anything that would attempt to *compile* it with these flags enabled, that would fail. So better remove the features entirely as to not to lie about what is available and what is not.

Patching "default" features should also be considered a "breaking" change, since it changes the behaviour of the Rust crate as packaged compared to the one from crates.io, so that should usually be avoided. In this case, it looks more or less unavoidable, because it would pull in a lot more backends / features / dependencies.

It looks like sccache isn't used by any other project as a library. So I would suggest that you just don't build those parts at all (i.e. set `cargo-install-lib = false` in rust2rpm.toml under the [package] table). This will make the packaging for this project a bit easier (and won't supply packages that aren't that useful).

3) You're passing the "-n" flag to %cargo_* macros, but this has no effect if the "default" feature has no dependencies (as is the case due to your patch). You should drop the "-n" flag, since it might cause issues in the future.

4) You're explicitly passing feature flags with "-f dist-client,memcached,native-zlib,webdav", which is a bit strange: You're patching *out* default features with the Cargo.toml patch, but patching them back *in* by passing feature flags on the command line - I would do one of them, not both (i.e. add the features you want enabled as dependencies of the "default" feature instead of the "all" feature). That way the spec file (and rust2rpm.toml config file) would be simpler.

5) You have a "manually created" patch that touches Cargo.toml. This is *not* supported by our tooling - all changes made to Cargo.toml must be made with "rust2rpm --patch", otherwise you might run into weird situations where the generated spec file doesn't match up with the package contents. You would need to apply the bump for the "object" dependency in the same patch as the other Cargo.toml changes. (Yes, I know it is a bit awkward to have to put unrelated changes into a single patch file.)

6) Your ExcludeArch setting is reduntant - drop %{arm} from it. 32-bit ARM hasn't been a supported architecture in Fedora for YEARS. The only 32-bit platform left is i686. And for just using "ExcludeArch: %{ix86}", you don't even need to add a comment or documentation since https://fedoraproject.org/wiki/Changes/EncourageI686LeafRemoval

7) The project contains some source files that aren't just "Apache-2.0":

- src/lru_disk_cache/lru_cache.rs is Apache-2.0 OR MIT
- tests/cmake-hip/vectoradd_hip.cpp is MIT

The latter looks like it could be excluded from built packages so it shouldn't matter much (especially if you decide to not ship any -devel subpackages as I suggested in point 2). But the first one needs to be documented and reflected in the spec file.

Spec URL: https://asn.fedorapeople.org/rust-sccache.spec
SRPM URL: https://asn.fedorapeople.org/rust-sccache-0.10.0-5.fc43.src.rpm

Thanks for the review:

1) Fixed
2) Dropped library, modified Cargo.toml for dropping dist-server and rouille
3) Modified default feature in Cargo.toml
4) Removed, fixed in Cargo.toml now
5) Edited patch moved to rust2rpm generated patch
6) Fixed
7) Fixed

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9048846
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2362051-rust-sccache/fedora-rawhide-x86_64/09048846-rust-sccache/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/

Please know that there can be false-positives.

---
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.

The FIXME for 1) said to use the output of "%cargo_license_summary", which is:

# (Apache-2.0 OR MIT) AND BSD-3-Clause
# (MIT OR Apache-2.0) AND Unicode-DFS-2016
# 0BSD OR MIT OR Apache-2.0
# Apache-2.0
# Apache-2.0 AND ISC AND (MIT OR Apache-2.0)
# Apache-2.0 OR Apache-2.0 WITH LLVM-exception
# Apache-2.0 OR BSL-1.0
# Apache-2.0 OR ISC OR MIT
# Apache-2.0 OR MIT
# Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT
# BSD-2-Clause
# BSD-2-Clause OR Apache-2.0 OR MIT
# BSD-3-Clause
# CDLA-Permissive-2.0
# ISC
# MIT
# MIT OR Apache-2.0
# MIT OR Zlib OR Apache-2.0
# MIT-0 OR Apache-2.0
# MPL-2.0
# Unicode-3.0
# Unlicense OR MIT
# Zlib

These all need to be covered by the License tag. It currently only lists:

> # src/lru_disk_cache/lru_cache.rs is Apache-2.0 OR MIT
> License:        Apache-2.0 OR MIT

Which should be the comment and change for the License tag for the *source package* (line 15) not the *built package* (line 45), and it should be something like this instead (just "Apache-2.0 OR MIT" is wrong in this situation):

> # most of sccache is Apache-2.0 except for
> # src/lru_disk_cache/lru_cache.rs which is Apache-2.0 OR MIT
> License: Apache-2.0 AND (Apache-2.0 OR MIT)

Also, part of the patch for Cargo.toml that doesn't touch the default features now should be droppable since you no longer ship the "-devel" subpackages, but that's not a blocker, just a possible simplification for the future that can be done after the review is done.

That means the License tag should be:

License: (Apache-2.0 OR MIT) AND Apache-2.0 AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND 0BSD AND BSD-2-Clause AND BSD-3-Clause AND BSL-1.0 AND CDLA-Permissive-2.0 AND ISC AND MIT AND MIT-0 AND MPL-2.0 AND Unicode-DFS-2016 AND Unicode-3.0 AND (Unlicense OR MIT) AND Zlib

Do you agree?

Looks incomplete .. a few clauses are definitely missing, and you simplified too much (0BSD doesn't occur on its own, it's only part of an OR expression that can't be dissolved into ANDs).

According to guidance I got from RH legal, the only "simplifications" that are valid are

- reordering within AND and OR collections (i.e. "A AND B" and "B AND A" are considered equivalent)
- dropping of parentheses for equivalent operators / associativity ("A AND (B AND C)" and "(A AND B) AND C" are equivalent to "A AND B AND C")

Notably, alternatives like "0BSD OR MIT OR Apache-2.0" need to be preserved.

With this in mind, this is the expression I get (ordered alphabetically and non-parenthesized items first):

License:        %{shrink:
    Apache-2.0 AND
    BSD-2-Clause AND
    BSD-3-Clause AND
    CDLA-Permissive-2.0 AND
    ISC AND
    MIT AND
    MPL-2.0 AND
    Unicode-3.0 AND
    Unicode-DFS-2016 AND
    Zlib AND
    (0BSD OR MIT OR Apache-2.0) AND
    (Apache-2.0 OR Apache-2.0 WITH LLVM-exception) AND
    (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND
    (Apache-2.0 OR BSL-1.0) AND
    (Apache-2.0 OR ISC OR MIT) AND
    (Apache-2.0 OR MIT) AND
    (BSD-2-Clause OR Apache-2.0 OR MIT) AND
    (MIT OR Zlib OR Apache-2.0) AND
    (MIT-0 OR Apache-2.0) AND
    (Unlicense OR MIT)
}

Spec URL: https://asn.fedorapeople.org/rust-sccache.spec
SRPM URL: https://asn.fedorapeople.org/rust-sccache-0.10.0-6.fc43.src.rpm

Created attachment 2093553 [details]
The .spec file difference from Copr build 9048846 to 9150060

Copr build:
https://copr.fedorainfracloud.org/coprs/build/9150060
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2362051-rust-sccache/fedora-rawhide-x86_64/09150060-rust-sccache/fedora-review/review.txt

Found issues:

- No gcc, gcc-c++ or clang found in BuildRequires
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/C_and_C++/
- Not a valid SPDX expression 'Apache-2.0 AND BSD-2-Clause AND BSD-3-Clause AND CDLA-Permissive-2.0 AND ISC AND MIT AND MPL-2.0 AND Unicode-3.0 AND Unicode-DFS-2016 AND Zlib AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR Apache-2.0 WITH LLVM-exception) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Zlib OR Apache-2.0) AND (MIT-0 OR Apache-2.0) AND (Unlicense OR MIT)'.
  Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1

Please know that there can be false-positives.

---
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.

If I run

license-validate --verbose 'Apache-2.0 AND BSD-2-Clause AND BSD-3-Clause AND CDLA-Permissive-2.0 AND ISC AND MIT AND MPL-2.0 AND Unicode-3.0 AND Unicode-DFS-2016 AND Zlib AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR Apache-2.0 WITH LLVM-exception) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Zlib OR Apache-2.0) AND (MIT-0 OR Apache-2.0) AND (Unlicense OR MIT)'

it complains about at C of CDLA-Permissive-2.0


No terminal matches 'C' in the current parser context, at line 1 col 50

0 AND BSD-2-Clause AND BSD-3-Clause AND CDLA-Permissive-2.0 AND ISC AND MIT AND

(In reply to Andreas Schneider from comment #14)
> it complains about at C of CDLA-Permissive-2.0

This license was *very* recently approved (https://gitlab.com/fedora/legal/fedora-license-data/-/issues/656) and added to fedora-license-data (https://gitlab.com/fedora/legal/fedora-license-data/-/merge_requests/756). You will need fedora-license-data 1.68 for an expression containing it to validate. Try:

   sudo dnf --enablerepo=updates-testing update fedora-license-data

With updated license data I get:

license-validate --verbose 'Apache-2.0 AND BSD-2-Clause AND BSD-3-Clause AND CDLA-Permissive-2.0 AND ISC AND MIT AND MPL-2.0 AND Unicode-3.0 AND Unicode-DFS-2016 AND Zlib AND (0BSD OR MIT OR Apache-2.0) AND (Apache-2.0 OR Apache-2.0 WITH LLVM-exception) AND (Apache-2.0 WITH LLVM-exception OR Apache-2.0 OR MIT) AND (Apache-2.0 OR BSL-1.0) AND (Apache-2.0 OR ISC OR MIT) AND (Apache-2.0 OR MIT) AND (BSD-2-Clause OR Apache-2.0 OR MIT) AND (MIT OR Zlib OR Apache-2.0) AND (MIT-0 OR Apache-2.0) AND (Unlicense OR MIT)'

Approved license

Thanks, package looks good now, with one exception:

Only the one in the "sccache" package itself needs to reflect the statically linked deps. The top-level License tag should only reflect the actual code contents in the source package (which is "Apache-2.0 OR MIT", as before).