Bug 2233248

Summary: Review Request: rust-glycin - Sandboxed image rendering
Product: [Fedora] Fedora Reporter: Kalev Lember <klember>
Component: Package ReviewAssignee: Fabio Valentini <decathorpe>
Status: CLOSED ERRATA QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: medium    
Version: rawhideCC: decathorpe, package-review
Target Milestone: ---Flags: decathorpe: fedora-review+
Target Release: ---   
Hardware: All   
OS: Linux   
URL: https://crates.io/crates/glycin
Whiteboard:
Fixed In Version: Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2023-08-21 21:18:37 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On: 2233226    
Bug Blocks:    
Attachments:
Description Flags
The .spec file difference from Copr build 6329802 to 6329914 none

Description Kalev Lember 2023-08-21 19:12:33 UTC
Spec URL: https://kalev.fedorapeople.org/rust-glycin.spec
SRPM URL: https://kalev.fedorapeople.org/rust-glycin-0.1.0~beta.3-1.fc40.src.rpm
Description:
Sandboxed image rendering.

Fedora Account System Username: kalev

Comment 1 Kalev Lember 2023-08-21 19:15:28 UTC
There is something weird going on with this package that I don't understand. rust2rpm generates a "%package -n %{crate}" subpackage with /usr/bin/test in it. I've neutered that one for now with the following: https://paste.centos.org/view/73e8dd81 , but I'm unsure what's the root cause and how to stop that from happening. I don't even know what to ask upstream -- is it something that's going wrong with rust2rpm, or is it something that's wrong in upstream set up?

Comment 2 Fedora Review Service 2023-08-21 19:20:28 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6329802
(failed)

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

Comment 3 Fabio Valentini 2023-08-21 20:10:51 UTC
Two minor issues:

1. "LGPL-2.1" is a deprecated SPDX license identifier. It should likely be "LGPL-2.1-only" or "LGPL-2.1-or-later", but I'm not sure which one.
Looks like I missed this in the review for glycin-utils, which has the same issue. :(

2. You can add "autobins = false" to the "[package]" section of Cargo.toml to prevent automatic detection of binaries, that would save you from doing this manually in %prep. It's not even a test (i.e. it's a [[bin]] target, not a [[test]] target), which is why it gets detected as an installable executable by cargo. If it *were* a proper [[test]] target, having a main() function would be perfectly fine. But this looks like a development / debugging tool, so setting "autobins = false" is likely the easiest solution to prevent it from being built.

Doing "rm src/bin/test.rs" is fine as well, but doing the autobins patch has the benefit of also making rust2rpm properly recognize this is a library-only / no debug symbols / no "binary package" package, so you wouldn't need to make all these modifications manually in addition to doing the rm-ing.

Comment 4 Kalev Lember 2023-08-21 20:42:22 UTC
Thanks! I updated it to use the more restrictive LGPL-2.1-only for now, and filed upstream issue https://gitlab.gnome.org/sophie-h/glycin/-/issues/15 for clarification. I'll do that same in glycin-utils as well.

As for the [[bin]] vs [[test]] target, where is it defined? I couldn't find [[bin]] string anywhere when grepping the source tree. Is it somehow automagically detected?

I've added a downstream patch with "autobins = false" for now, which seems to nicely fix this. Or is it something that can go upstream as well? Sorry, clueless about rust here :)

Spec URL: https://kalev.fedorapeople.org/rust-glycin.spec
SRPM URL: https://kalev.fedorapeople.org/rust-glycin-0.1.0~beta.3-3.fc40.src.rpm

Comment 5 Fedora Review Service 2023-08-21 20:44:52 UTC
Created attachment 1984455 [details]
The .spec file difference from Copr build 6329802 to 6329914

Comment 6 Fedora Review Service 2023-08-21 20:44:55 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6329914
(failed)

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

Comment 7 Fabio Valentini 2023-08-21 20:49:54 UTC
If you lay out your project's directory structure in the conventional way, cargo will auto-detect binaries, tests, examples, benchmarks for you, without having to list them all manually in Cargo.toml with [[bin]], [[example]], [[test]], or [[bench]] targets. You only need to manually specify these compilation targets if they need non-default settings: https://doc.rust-lang.org/cargo/reference/cargo-targets.html#target-auto-discovery

In this case: Files that match src/bin/*.rs (the convention for projects that ship multiple executables) are automatically detected as executables - as would be src/main.rs (the convention for single-executable projects).

Hope that makes sense.

Package looks good to me now!

===

Package was generated with rust2rpm, simplifying the review.

- 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 (LGPL-2.1-*) 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:

- 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

- add @rust-sig with "commit" access as package co-maintainer
  (should happen automatically)

- set bugzilla assignee overrides to @rust-sig (optional)

- track package in koschei for all built branches
  (should happen automatically once rust-sig is co-maintainer)

===

Note that %cargo_license and %cargo_license_summary macros operate on cargo metadata - so for these to pick up the fixed license identifier correctly, you would also need to patch it in Cargo.toml (in addition to the "autobins = false" thing).

Comment 8 Kalev Lember 2023-08-21 20:55:35 UTC
Ohh, thanks, let me fix the license in Cargo.toml as well then. Thanks for the review and all the clarification about executables! I'll discuss this with upstream tomorrow.

Comment 9 Fedora Admin user for bugzilla script actions 2023-08-21 20:56:41 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-glycin

Comment 10 Fedora Update System 2023-08-21 21:17:19 UTC
FEDORA-2023-d8bdec0a06 has been submitted as an update to Fedora 39. https://bodhi.fedoraproject.org/updates/FEDORA-2023-d8bdec0a06

Comment 11 Fedora Update System 2023-08-21 21:18:37 UTC
FEDORA-2023-d8bdec0a06 has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 12 Fedora Update System 2023-08-21 21:22:00 UTC
FEDORA-2023-3179da3f6a has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-3179da3f6a

Comment 13 Fedora Update System 2023-08-21 21:24:40 UTC
FEDORA-2023-3179da3f6a has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.