Bug 2334841 - Review Request: rust-libphosh-sys - Rust bindings for libphosh
Summary: Review Request: rust-libphosh-sys - Rust bindings for libphosh
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
unspecified
medium
Target Milestone: ---
Assignee: Fabio Valentini
QA Contact: Fedora Extras Quality Assurance
URL: https://crates.io/crates/libphosh-sys
Whiteboard:
Depends On:
Blocks: 2334842
TreeView+ depends on / blocked
 
Reported: 2024-12-29 17:12 UTC by Sam Day
Modified: 2025-01-21 14:30 UTC (History)
2 users (show)

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2025-01-12 19:07:57 UTC
Type: ---
Embargoed:
decathorpe: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 8453909 to 8453952 (548 bytes, patch)
2024-12-29 18:14 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 8453952 to 8465518 (1.39 KB, patch)
2025-01-02 21:20 UTC, Fedora Review Service
no flags Details | Diff
The .spec file difference from Copr build 8465518 to 8500328 (2.17 KB, patch)
2025-01-10 21:41 UTC, Fedora Review Service
no flags Details | Diff

Description Sam Day 2024-12-29 17:12:45 UTC
Spec URL: https://pagure.io/rust-libphosh-sys/blob/main/f/rust-libphosh-sys.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/4120/127374120/rust-libphosh-sys-0.0.4-2.fc42.src.rpm
Description: Rust bindings for libphosh
Fedora Account System Username: samcday


https://koji.fedoraproject.org/koji/taskinfo?taskID=127374098

This package alone isn't very useful, it's primarily consumed by the main `libphosh` crate - as such I will be submitting the `rust-libphosh` package shortly after this submission.

I am not yet a packager, and would like to be sponsored. In addition to `rust-libphosh-sys` + `rust-libphosh`, I intend to also submit packaging for `phrog` (https://github.com/samcday/phrog), which makes use of these proposed packages.

Comment 1 Fedora Review Service 2024-12-29 17:19:57 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8453909
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2334841-rust-libphosh-sys/fedora-rawhide-x86_64/08453909-rust-libphosh-sys/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.

Comment 3 Fedora Review Service 2024-12-29 18:14:56 UTC
Created attachment 2064159 [details]
The .spec file difference from Copr build 8453909 to 8453952

Comment 4 Fedora Review Service 2024-12-29 18:14:58 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8453952
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2334841-rust-libphosh-sys/fedora-rawhide-x86_64/08453952-rust-libphosh-sys/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.

Comment 5 Fabio Valentini 2025-01-02 17:38:25 UTC
Looks mostly OK, with some minor things:

0. Don't mix tabs and spaces in the same file. :)

1. Why did you manually add "BuildRequires: gcc"? If it's to placate some warning, you can ignore it - the Rust compiler already hard-codes a dependency on gcc.

2. "ExcludeArch: i686" is better written as "ExcludeArch: %{ix86}" to avoid problems with systems that define this differently (it's i686 in koji but i386 in COPR, IIRC).

3. You also need to add "Requires: libphosh-devel" to the "devel" subpackage, otherwise it won't be pulled in when building packages that depend on libphosh-sys.

4. Replace "%license LICENSE" with "%license %{crate_instdir}/LICENSE" to avoid including the LICENSE file in built packages twice.

You might be interested in also adding a rust2rpm.toml config file to automate some parts of this package.

For example, this snippet would cause the dependency on libphosh-devel to be automatically injected where required:

```
[requires]
build = ["libphosh-devel"]
lib = ["libphosh-devel"]
```

Comment 6 Sam Day 2025-01-02 20:05:10 UTC
0. Oops xD

1. Yeah I was mostly trying to placate fedora-review-service. I suspected something like what you're saying, but couldn't see the compiler dep when I went looking at the rust package. Guess I just didn't look carefully enough ;) I'll remove this unnecessary dep in the next revision.

2. Interesting, guess I didn't run into the inconsistency because I've only been building against aarch64/x64 in COPR

3. Ah, right. Oops! I'll add a rust2rpm.toml per your suggestion and fix this up, thanks.

4. I see, I'll do that in the next revision. Interestingly I've already done this in some local changes that build rust-libphosh-sys 0.0.5, but I can't submit that yet because libphosh 0.44 hasn't made its way into the repos.

I'll push up a new revision soon, thanks for the review!

Comment 7 Sam Day 2025-01-02 21:11:39 UTC
Spec URL: https://pagure.io/rust-libphosh-sys/raw/94440753295a75b0caecce51151b2cef02a6d032/f/rust-libphosh-sys.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/8055/127498055/rust-libphosh-sys-0.0.4-3.fc42.src.rpm

---

Notes:

 * I've added a basic [rust2rpm.toml](https://pagure.io/rust-libphosh-sys/blob/main/f/rust2rpm.toml) with the suggested content.
 * I didn't bother to incorporate the manually patched LICENSE Source: + script to copy that file to rust2rpm.toml, because those will be removed in libphosh 0.0.5 (which includes the LICENSE in the packaged crate on crates.io)
 * As noted in previous comment, I can't do the 0.0.5 bump yet because it depends on Phosh being updated to 0.44 first.
 * I couldn't see a way to add ExcludeArch in rust2rpm.toml. `[package.supported-arches]` seems to be a different thing, right?
 * While investigating the ExcludeArch thing, I came across one of [your comments last year](https://bugzilla.redhat.com/show_bug.cgi?id=2266544#c5) indicating that a comment as to why i686 isn't necessary - so I took some initiative and removed the ExcludeArch comment.

Comment 8 Fedora Review Service 2025-01-02 21:20:17 UTC
Created attachment 2064551 [details]
The .spec file difference from Copr build 8453952 to 8465518

Comment 9 Fedora Review Service 2025-01-02 21:20:20 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8465518
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2334841-rust-libphosh-sys/fedora-rawhide-x86_64/08465518-rust-libphosh-sys/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.

Comment 10 Fabio Valentini 2025-01-10 20:49:33 UTC
Oh Oh. looks like phosh was updated to v0.44 (by yourself? https://src.fedoraproject.org/rpms/phosh/pull-request/3), which breaks building the bindings:

> The system library `libphosh-0.43` required by crate `libphosh-sys` was not found.

Comment 11 Sam Day 2025-01-10 21:35:54 UTC
Ah, yes - I just submitted the PR to help out my homie lihis, who is maintaining the Phosh packages in Fedora. I wasn't keeping an eye on when that stuff was merging or grinding through Bodhi :)

Anyways, here's the next revision. I've changed the build dep to `pkgconfig(libphosh-0.44)` to make it clear that these crates are currently pinned to a rather precise version of the libphosh API (which hasn't stabilized yet, but we're hoping to nail down some stuff for 0.45). I was also able to remove the LICENSE hax as promised earier.

---

Spec URL: https://pagure.io/rust-libphosh-sys/raw/b6b6acaeb9d2522960db07586fd4f43a646c8d11/f/rust-libphosh-sys.spec
SRPM URL: https://kojipkgs.fedoraproject.org//work/tasks/9769/127749769/rust-libphosh-sys-0.0.5-1.fc42.src.rpm

Koji scratch: https://koji.fedoraproject.org/koji/taskinfo?taskID=127749768

Comment 12 Fedora Review Service 2025-01-10 21:41:14 UTC
Created attachment 2065433 [details]
The .spec file difference from Copr build 8465518 to 8500328

Comment 13 Fedora Review Service 2025-01-10 21:41:17 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/8500328
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2334841-rust-libphosh-sys/fedora-rawhide-x86_64/08500328-rust-libphosh-sys/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.

Comment 14 Fabio Valentini 2025-01-12 17:04:34 UTC
Thanks! This looks good to me now.

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 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 (*NOT* pre-release 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)

Comment 15 Fedora Admin user for bugzilla script actions 2025-01-12 18:39:59 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-libphosh-sys

Comment 16 Fedora Update System 2025-01-12 19:03:28 UTC
FEDORA-2025-a65f973ead (rust-libphosh-sys-0.0.5-1.fc42) has been submitted as an update to Fedora 42.
https://bodhi.fedoraproject.org/updates/FEDORA-2025-a65f973ead

Comment 17 Fedora Update System 2025-01-12 19:07:57 UTC
FEDORA-2025-a65f973ead (rust-libphosh-sys-0.0.5-1.fc42) has been pushed to the Fedora 42 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 18 Fabio Valentini 2025-01-15 14:31:50 UTC
Looks like you figured out all the necessary steps to land this in the repos - congratulations on your first official package :)
Let me (or Neal) know if you need any help or have any questions. We're both in the #rust:fedoraproject.org room on Matrix.

Comment 19 Sam Day 2025-01-15 14:59:33 UTC
Yes indeed! I followed the excellent documentation and marvelled at how all the automated tooling and systems worked exactly the way the docs said they would. As a jaded/grizzled Software Person I was sure that literally never actually happens these days ;)

Thanks for all the help you've provided thus far - answering my questions, reviewing these packages, and bringing back rust-libhandy <3

Comment 20 Fabio Valentini 2025-01-21 14:30:38 UTC
(In reply to Sam Day from comment #19)
> Yes indeed! I followed the excellent documentation and marvelled at how all
> the automated tooling and systems worked exactly the way the docs said they
> would. As a jaded/grizzled Software Person I was sure that literally never
> actually happens these days ;)

There *are* some outdated docs out there, for sure, but at least for common tasks there's a few people who work to keep things up-to-date.

> Thanks for all the help you've provided thus far - answering my questions,
> reviewing these packages, and bringing back rust-libhandy <3

No problem! Looking forward to seeing phrog.


Note You need to log in before you can comment on or make changes to this bug.