Spec URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4.spec SRPM URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4-0.10.10-1.fc40.src.rpm Description: GStreamer GTK 4 Sink element and Paintable widget. Fedora Account System Username: kalev
Hm, this currently doesn't build, because it depends on gdk4-wayland / gdk4-x11 v0.6, but we have v0.7 in Fedora. It might be better to update all the GStreamer bindings and package the latest version of gst-plugin-gtk4 than to create even more compat packages?
Ah sorry, I already requested the missing compat packages and the builds just finished for rawhide and f39-build-side-74672
*sigh* oh, well I'll finish up the review tomorrow then.
Awesome! I really appreciate all the reviews.
Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106862047
Now that the rust gstreamer bindings are updated to 0.21.x, I was able to update gst-plugin-gtk4 to the latest version as well: * Fri Sep 29 2023 Kalev Lember <klember> - 0.11.0-1 - Update to 0.11.0 - Downgrade cargo-c dep to the version available in Fedora Spec URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4.spec SRPM URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4-0.11.0-1.fc40.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106885848
Some minor issues: 1. The License tag for the "source" package in line 26 should only contain the license of the project itself (MPL-2.0). The licenses for statically linked Rust dependencies should only go into the License tag for the gstreamer1-plugin-gtk4 subpackage. 2. You can use the %cargo_cbuild and %cargo_install macros instead of using the "private" %__cargo macro to manually call these commands. I recently added them to the cargo-c package and haven't yet had the opportunity to switch the gst-plugin-reqwest package to them. 3. Why do you use the `-a` (--all-features) flag for %cargo_generate_buildrequires / %cargo_build / %cargo_install / %cargo_test? It doesn't look like the shared object is built with "--all-features", so I don't see why it should be necessary. Or did you *want* to build the plugin with all features enabled? Then the "--all-features" is missing from cbuild / cinstall calls *and* from the %cargo_license_summary / %cargo_license macro calls. 4. It would be great if you could add virtual Provides for the upstream name to the plugin subpackage as well, similar to what we've done in gst-plugin-reqwest. The naming convention for GStreamer plugins in Fedora is rather weird since it doesn't match upstream names at all, so providing the upstream name can prevent some confusion here, I think.
(In reply to Fabio Valentini from comment #7) > Some minor issues: > > 1. The License tag for the "source" package in line 26 should only contain > the license of the project itself (MPL-2.0). > The licenses for statically linked Rust dependencies should only go into the > License tag for the gstreamer1-plugin-gtk4 subpackage. Ohh, oops, I had added it to the wrong package. Fixed! > 2. You can use the %cargo_cbuild and %cargo_install macros instead of using > the "private" %__cargo macro to manually call these commands. > I recently added them to the cargo-c package and haven't yet had the > opportunity to switch the gst-plugin-reqwest package to them. Nice, I didn't know about the new macros! That makes it much cleaner. > 3. Why do you use the `-a` (--all-features) flag for > %cargo_generate_buildrequires / %cargo_build / %cargo_install / %cargo_test? > It doesn't look like the shared object is built with "--all-features", so I > don't see why it should be necessary. Or did you *want* to build the plugin > with all features enabled? Then the "--all-features" is missing from cbuild > / cinstall calls *and* from the %cargo_license_summary / %cargo_license > macro calls. I was working around a build failure without really understanding what --all-features does. Looks like updated 0.11.0 builds fine without it so I just went ahead and removed -a from everywhere :) I'll ask you if the issue should resurface. Not entirely sure what was going on there. > 4. It would be great if you could add virtual Provides for the upstream name > to the plugin subpackage as well, similar to what we've done in > gst-plugin-reqwest. The naming convention for GStreamer plugins in Fedora is > rather weird since it doesn't match upstream names at all, so providing the > upstream name can prevent some confusion here, I think. Sure, added. * Fri Sep 29 2023 Kalev Lember <klember> - 0.11.0-2 - Package review fixes (#2241245) - Fix source package license to only contain the license of the project itself - Add virtual provides for gst-plugin-gtk4 to gstreamer1-plugin-gtk4 subpackage - Use cargo_cbuild and cargo_cinstall macros - Drop inadvertent --all-features flag Spec URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4.spec SRPM URL: https://kalev.fedorapeople.org/rust-gst-plugin-gtk4-0.11.0-2.fc40.src.rpm Koji scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=106892373 Diff with the latest changes if it makes it easier for you to review changes: https://paste.centos.org/view/97e99661
Thanks, the diff makes it easy indeed. Package looks good to me (not pasting the complete checklist here since it would now be all [x]s anyway). APPROVED
Thanks a lot for the review!
The Pagure repository was created at https://src.fedoraproject.org/rpms/rust-gst-plugin-gtk4
Package imported and built.