Spec URL: https://elmarco.fedorapeople.org/rutabaga-gfx-ffi.spec SRPM URL: https://elmarco.fedorapeople.org/rutabaga-gfx-ffi-0.1.2-1.20230913gitc3ad0e43e.fc40.src.rpm Description: A library for handling 2D and 3D virtio-gpu hypercalls Fedora Account System Username: elmarco
there is an issue with the library SONAME
Just from a Rust packaging standpoint, there's two immediate red flags: 1. There's a "--no-default-features" flag passed to "%cargo_generate_buildrequires -n", but the "--all-features" flag (pretty much the *opposite one*) is passed to %cargo_license/_summary ("-a"). This is likely to cause wrong results. You should, in almost all cases, pass the same flags to *all* invocations of the %cargo_* macros, otherwise you will get inconsistent outcomes or weird errors. I don't see how "cargo build" is called inside the Makefile, but the same feature flags should also be passed to the %cargo_* macros in the spec. 2. You need to ensure that the correct compiler flags are passed to rustc. The fact that you have "%global debug_package %{nil}" probably means that the *wrong* flags are passed / hardcoded in the Makefile. If default RUSTFLAGS were respected, the output would have valid debuginfo. --- And just for better readability of the spec file, this is very confusing: > %prep > %autosetup -n rutabaga-gfx-%{gitdate} -p1 > %cargo_prep > %generate_buildrequires > %cargo_generate_buildrequires -n The %generate_buildrequires scriptlet is a top-level scriptlet just like %prep, %build, and %install, having it smushed together with %prep to make it look like it's part of %prep is likely to cause confusion. --- You might also get away with using "patchelf" to set the soname instead of patching in a custom build script.
thanks Fabio for the quick review! 1. ok I'll fix the flags "cargo build" is called from the "build" Makefile target. I suppose we could replace it with %cargo_* macros (as long as I can specify the ffi subdirectory) 2. ok, hopefully that will fix it! I'll move the generate_buildrequires. Regarding the soname, I submitted a patch to one of the maintainer (Gurchetan). He is usually quick at applying the fixes upstream.
Thanks! I'll take another look later. Note that the project's directory structure / setup is a bit unusual. It looks like most of the functionality of this project is disabled by default: > [features] > gfxstream = [] > gfxstream_stub = [] > virgl_renderer = [] > virgl_renderer_next = [] > minigbm = [] > # To try out Vulkano, delete the following line and uncomment the line in "dependencies". Vulkano > # features are just a prototype and not integrated yet into the ChromeOS build system. > vulkano = [] > x = [] The only feature that's enabled conditionally by the Makefile is gfxstream if the corresponding pkg-config file can be found, and this library doesn't appear to be packaged for Fedora. So I think in fact, you don't need to pass *any* flags to "%cargo_generate_buildrequires", "%cargo_license", or "%cargo_license_summary", since there are not manually enabled features, and the default feature set is empty. === Looking at the whole thing, I think it would be easier to just not use the Makefile at all than to work around its limitations, especially if you need to make any adjustments to the build process (like enabling any of the features, or passing other flags).
- generate_buildrequires needs to be during %prep, because sources must be extracted (that's also how rust-ripgrep does) - fixed the -a/-n flags - %cargo_build ok, however debugsourcefiles.list is still empty (breaking the build atm). Any idea how to fix find-debuginfo ?
> - generate_buildrequires needs to be during %prep, because sources must be extracted (that's also how rust-ripgrep does) This is a misunderstanding of how RPM works. You could also write this: """ %generate_buildrequires %cargo_generate_buildrequires -n %prep %autosetup -n rutabaga-gfx-%{gitdate} -p1 %cargo_prep """ And the %prep scriptlet would still get executed *before* the %generate_buildrequires scriptlet. Order of definition in the spec file does not matter to RPM, it will always execute scriptlets in the same order. > - fixed the -a/-n flags Why did you keep the "-n" flag? It has zero effect in this project. > - %cargo_build ok, however debugsourcefiles.list is still empty (breaking the build atm). Any idea how to fix find-debuginfo ? This means that something is wrong during the build process - the binaries end up without debug information, otherwise find-debuginfo would not complain. I don't know how this happens, but the build log should show "-Cdebuginfo=2" being passed to rustc. If that doesn't happen, then something is wrong.
(In reply to Fabio Valentini from comment #6) > > - generate_buildrequires needs to be during %prep, because sources must be extracted (that's also how rust-ripgrep does) > > This is a misunderstanding of how RPM works. You could also write this: > > """ > %generate_buildrequires > %cargo_generate_buildrequires -n > > %prep > %autosetup -n rutabaga-gfx-%{gitdate} -p1 > %cargo_prep > """ > > And the %prep scriptlet would still get executed *before* the > %generate_buildrequires scriptlet. Order of definition in the spec file does > not matter to RPM, it will always execute scriptlets in the same order. ok > > > - fixed the -a/-n flags > > Why did you keep the "-n" flag? It has zero effect in this project. You are right, I was confused. > > > - %cargo_build ok, however debugsourcefiles.list is still empty (breaking the build atm). Any idea how to fix find-debuginfo ? > > This means that something is wrong during the build process - the binaries > end up without debug information, otherwise find-debuginfo would not > complain. > I don't know how this happens, but the build log should show "-Cdebuginfo=2" > being passed to rustc. If that doesn't happen, then something is wrong. fixed now
Thanks for the update, and sorry for the wait. Package looks mostly good to me, there's just a few remaining issues: 1. The "make-snapshot.sh" script is not included in the SRPM. It should be included as a SOURCE, otherwise it's impossible to re-create the srpm from its contents. 2. The %cargo_license macro has an "-a" flag (for "--all-features"), which will make its results include things that aren't actually linked into the binary. Please drop the "-a" flag here. 3. The crates do not set their license in crate metadata, which trips up the %cargo_license* macros. It would be great if upstream set `package.license = "BSD-3-Clause"` for both crates ... but until that's the case, it might be a good idea to patch that in manually so the macros produce correct / complete output. 4. The licenses of all statically linked dependencies are listed in LICENSE.dependencies, but the License tag itself does not reflect them. Looking at the contents of the LICENSE.dependencies file from the built package, the License tag in the spec should look something like this: """ # rutabaga_gfx: BSD-3-Clause # crate dependencies: # - BSD-2-Clause # - MIT OR Apache-2.0 # - MIT # - Unlicense OR MIT License: BSD-3-Clause AND BSD-2-Clause AND MIT AND (MIT OR Apache-2.0) AND (Unlicense OR MIT) """ 5. The patches don't have explanations or justifications, please add a short comment for each one, like """ # drop a Windows-specific dependency Patch0000: drop-winapi.patch # fix filename for installed library and set up symlinks correctly Patch0001: fix-libs.patch # set library SONAME correctly Patch0002: set-soname.patch """ (I'm assuming that's the reason for the patches, but I'm just guessing, since they're not documented yet :)) From what it looks like, the "fix-libs.patch" could be dropped entirely, since "make" is no longer used to install the files? 7. You could drop the BuildRequires for "make" since it is no longer used. 8. There's no URL to the upstream project in the package metadata. I would add this instead of only having the url in a comment: URL: https://chromium.googlesource.com/crosvm/crosvm 9. The placement of the "%generate_buildrequires" scriptlet (between RPM tags and %description) is very unusual. Please place it between %prep and %build (since that's the order in which the scriptlets are executed). 10. Please bump the BuildRequires from "rust-packaging >= 21" to "cargo-rpm-macros". The "rust-packaging" package no longer exists and is only provided by "cargo-rpm-macros" for backwards compatibility.
Oh, and another one: 11. Don't use a broad glob to list the shared library in %files: https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files i.e. replace the line %{_libdir}/librutabaga_gfx_ffi.so.* with %{_libdir}/librutabaga_gfx_ffi.so.0{,.*}
(In reply to Fabio Valentini from comment #9) > i.e. replace the line > %{_libdir}/librutabaga_gfx_ffi.so.* > with > %{_libdir}/librutabaga_gfx_ffi.so.0{,.*} done
(In reply to Fabio Valentini from comment #8) > Thanks for the update, and sorry for the wait. No worries (the QEMU side isn't yet merged upstream, and I am not sure when we will actually have users) > 1. The "make-snapshot.sh" script is not included in the SRPM. It should be > included as a SOURCE, otherwise it's impossible to re-create the srpm from > its contents. fixed > > 2. The %cargo_license macro has an "-a" flag (for "--all-features"), which > will make its results include things that aren't actually linked into the > binary. Please drop the "-a" flag here. ok > > 3. The crates do not set their license in crate metadata, which trips up the > %cargo_license* macros. It would be great if upstream set `package.license = > "BSD-3-Clause"` for both crates ... but until that's the case, it might be a > good idea to patch that in manually so the macros produce correct / complete > output. It seems they considered it: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4082335 I don't see what difference there is between the Chromium LICENSE and BSD-3 though. > > 4. The licenses of all statically linked dependencies are listed in > LICENSE.dependencies, but the License tag itself does not reflect them. > Looking at the contents of the LICENSE.dependencies file from the built > package, the License tag in the spec should look something like this: > > """ > # rutabaga_gfx: BSD-3-Clause > # crate dependencies: > # - BSD-2-Clause > # - MIT OR Apache-2.0 > # - MIT > # - Unlicense OR MIT > License: BSD-3-Clause AND BSD-2-Clause AND MIT AND (MIT OR > Apache-2.0) AND (Unlicense OR MIT) > """ fixed, thanks > > 5. The patches don't have explanations or justifications, please add a short > comment for each one, like > > """ > # drop a Windows-specific dependency > Patch0000: drop-winapi.patch > # fix filename for installed library and set up symlinks correctly > Patch0001: fix-libs.patch > # set library SONAME correctly > Patch0002: set-soname.patch > """ done > > (I'm assuming that's the reason for the patches, but I'm just guessing, > since they're not documented yet :)) > From what it looks like, the "fix-libs.patch" could be dropped entirely, > since "make" is no longer used to install the files? done > > 7. You could drop the BuildRequires for "make" since it is no longer used. ok > > 8. There's no URL to the upstream project in the package metadata. > > I would add this instead of only having the url in a comment: > URL: https://chromium.googlesource.com/crosvm/crosvm ok > > 9. The placement of the "%generate_buildrequires" scriptlet (between RPM > tags and %description) is very unusual. Please place it between %prep and > %build (since that's the order in which the scriptlets are executed). Hmm, I thought it was there initially and you asked me to move it. I realize now you just asked for an extra empty line, ok.. (confusing) > > 10. Please bump the BuildRequires from "rust-packaging >= 21" to > "cargo-rpm-macros". > The "rust-packaging" package no longer exists and is only provided by > "cargo-rpm-macros" for backwards compatibility. ok
> > 10. Please bump the BuildRequires from "rust-packaging >= 21" to > > "cargo-rpm-macros". > > The "rust-packaging" package no longer exists and is only provided by > > "cargo-rpm-macros" for backwards compatibility. > > ok I don't know what happened here, the new line is BuildRequires: rust-srpm-macros >= 24 - which is wrong. The rust-srpm-macros package is included in the default buildroot, and does not contain the cargo RPM macros. When I said to replace the "rust-packaging >= 21" BR with just "cargo-rpm-macros", I meant what I said 😅 Other than that, thank you for the update, looks good. Not sure if rutabaga-gfx-ffi or rutabaga-gfx would be the better package name (since the -ffi suffix is meaningless here) though, but that's your decision.
(In reply to Fabio Valentini from comment #12) > > > 10. Please bump the BuildRequires from "rust-packaging >= 21" to > > > "cargo-rpm-macros". > > > The "rust-packaging" package no longer exists and is only provided by > > > "cargo-rpm-macros" for backwards compatibility. > > > > ok > > I don't know what happened here, the new line is > > BuildRequires: rust-srpm-macros >= 24 > > - which is wrong. The rust-srpm-macros package is included in the default > buildroot, and does not contain the cargo RPM macros. > When I said to replace the "rust-packaging >= 21" BR with just > "cargo-rpm-macros", I meant what I said 😅 oh ok.. I screwed up. I couldn't easily find any rust package using that btw. fixed > > Other than that, thank you for the update, looks good. > Not sure if rutabaga-gfx-ffi or rutabaga-gfx would be the better package > name (since the -ffi suffix is meaningless here) though, but that's your > decision. Well, I named it after the lib and .pc, I think it's more straightforward this way.
> I couldn't easily find any rust package using that btw. fixed There's over 2000 Rust packages, and they are only being transitioned from BR: rust-packaging to BR: cargo-rpm-macros over time. > Well, I named it after the lib and .pc, I think it's more straightforward this way. Makes sense to me. Thanks! === I'd approve the package, but it doesn't build anymore: > error: no matching package named `once_cell` found > location searched: registry `crates-io` You need to run `%cargo_generate_buildrequires` inside the "ffi" directory, as far as I can tell, i.e. """ %generate_buildrequires cd ffi %cargo_generate_buildrequires cd .. """ It is *very* unusual to have the "workspace root" in a subpackage of the actual "project root" ... but that's an upstream problem. === And there's a warning because you now patch to set both package.license *and* package.license-file: > warning: /builddir/build/BUILD/rutabaga-gfx-20230913/Cargo.toml: only one of `license` or `license-file` is necessary > `license` should be used if the package license can be expressed with a standard SPDX expression. > `license-file` should be used if the package uses a non-standard license. > See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information.
(In reply to Fabio Valentini from comment #14) > > error: no matching package named `once_cell` found > > location searched: registry `crates-io` > > You need to run `%cargo_generate_buildrequires` inside the "ffi" directory, > as far as I can tell, i.e. > > """ > %generate_buildrequires > cd ffi > %cargo_generate_buildrequires > cd .. > """ ok > > It is *very* unusual to have the "workspace root" in a subpackage of the > actual "project root" ... but that's an upstream problem. We could eventually package the rust-rutabaga-gfx first, modify the -ffi and compile against it. Is that really preferable? > > === > > And there's a warning because you now patch to set both package.license > *and* package.license-file: > > > warning: /builddir/build/BUILD/rutabaga-gfx-20230913/Cargo.toml: only one of `license` or `license-file` is necessary > > `license` should be used if the package license can be expressed with a standard SPDX expression. > > `license-file` should be used if the package uses a non-standard license. > > See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information. hmm, I'll drop license-file in the patch then.
(In reply to Marc-Andre Lureau from comment #15) > (In reply to Fabio Valentini from comment #14) > > > error: no matching package named `once_cell` found > > > location searched: registry `crates-io` > > > > You need to run `%cargo_generate_buildrequires` inside the "ffi" directory, > > as far as I can tell, i.e. > > > > """ > > %generate_buildrequires > > cd ffi > > %cargo_generate_buildrequires > > cd .. > > """ > > ok You know, I *intentionally* wrote "cd ffi" and "cd .." instead of pushd / popd ... Because pushd and popd break the %generate_buildrequires script, since they print things to stdout (which is then interpreted as a dependency). > > > > It is *very* unusual to have the "workspace root" in a subpackage of the > > actual "project root" ... but that's an upstream problem. > > > We could eventually package the rust-rutabaga-gfx first, modify the -ffi and > compile against it. Is that really preferable? No. That's fine. I just wanted to note that the upstream directory layout is certainly "a choice" (a very weird one), which makes it a bit weird to build with cargo. > > > > === > > > > And there's a warning because you now patch to set both package.license > > *and* package.license-file: > > > > > warning: /builddir/build/BUILD/rutabaga-gfx-20230913/Cargo.toml: only one of `license` or `license-file` is necessary > > > `license` should be used if the package license can be expressed with a standard SPDX expression. > > > `license-file` should be used if the package uses a non-standard license. > > > See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information. > > hmm, I'll drop license-file in the patch then. Thanks, the license patch looks good to me.
(In reply to Fabio Valentini from comment #16) > (In reply to Marc-Andre Lureau from comment #15) > > (In reply to Fabio Valentini from comment #14) > > > > error: no matching package named `once_cell` found > > > > location searched: registry `crates-io` > > > > > > You need to run `%cargo_generate_buildrequires` inside the "ffi" directory, > > > as far as I can tell, i.e. > > > > > > """ > > > %generate_buildrequires > > > cd ffi > > > %cargo_generate_buildrequires > > > cd .. > > > """ > > > > ok > > You know, I *intentionally* wrote "cd ffi" and "cd .." instead of pushd / > popd ... > Because pushd and popd break the %generate_buildrequires script, since they > print things to stdout (which is then interpreted as a dependency). oh dear... > > > > > > > It is *very* unusual to have the "workspace root" in a subpackage of the > > > actual "project root" ... but that's an upstream problem. > > > > > > We could eventually package the rust-rutabaga-gfx first, modify the -ffi and > > compile against it. Is that really preferable? > > No. That's fine. I just wanted to note that the upstream directory layout is > certainly "a choice" (a very weird one), which makes it a bit weird to build > with cargo. > > > > > > > === > > > > > > And there's a warning because you now patch to set both package.license > > > *and* package.license-file: > > > > > > > warning: /builddir/build/BUILD/rutabaga-gfx-20230913/Cargo.toml: only one of `license` or `license-file` is necessary > > > > `license` should be used if the package license can be expressed with a standard SPDX expression. > > > > `license-file` should be used if the package uses a non-standard license. > > > > See https://doc.rust-lang.org/cargo/reference/manifest.html#the-license-and-license-file-fields for more information. > > > > hmm, I'll drop license-file in the patch then. > > Thanks, the license patch looks good to me. thanks
Thanks for the update. Can you verify that the new version builds?
Looks like there's still some weirdness going on due to the unusual directory / workspace setup in the upstream project ... You might need to run %cargo_generate_buildrequires twice, like this? """ %generate_buildrequires %cargo_generate_buildrequires cd ffi %cargo_generate_buildrequires cd .. """
(In reply to Fabio Valentini from comment #18) > Thanks for the update. Can you verify that the new version builds? it builds for me on f39 (rawhide toolbox is kinda broken for me atm, due to dnf5 transition)
Weird, dnf5 transition should have been reverted in both rawhide and f39. Maybe the toolbx containers haven't been rebuilt lately ... Either way, the package now looks good to me. Thank you for sticking with it through so many iterations! Package APPROVED. (I won't paste the full fedora-review checklist here, since we've basically gone over the whole thing manually already.) ============================================================ Some comments for after the package is imported: I don't know how you're going to keep track of upstream changes, since this is a git snapshot of some crosvm subproject, but it would be good to figure out a way to do that. I recommend turning on koschei tracking for this package after it's been imported. I also recommend adding "@rust-sig" with "commit" access to the package. It helps SIG members deal with changes that affect multiple packages (like porting packages to new versions of dependencies, or rebuilding packages for security vulnerabilities).
The Pagure repository was created at https://src.fedoraproject.org/rpms/rutabaga-gfx-ffi
I see that you tried building the package and got stopped by some changes I pushed to the cargo RPM macros a few days ago. Sorry about that! I pushed a followup fix to dist-git for that, the package now builds correctly (except that it fails to build on ppc64le and s390x). Not sure what this error is about: > error[E0433]: failed to resolve: use of undeclared crate or module `stdio` > --> /builddir/build/BUILD/rutabaga-gfx-20230913/src/generated/virgl_debug_callback_bindings.rs:70:66 > | > 70 | unsafe extern "C" fn(fmt: *const ::std::os::raw::c_char, ap: stdio::va_list), > | ^^^^^ use of undeclared crate or module `stdio` > For more information about this error, try `rustc --explain E0433`. > error: could not compile `rutabaga_gfx` (lib) due to previous error Looks like some APIs are not available on those two architectures? If you don't need rutabaga-gfx-ffi on ppc64le and s390x, I would just use "ExclusiveArch: x86_64 aarch64".
(In reply to Fabio Valentini from comment #23) > I see that you tried building the package and got stopped by some changes I > pushed to the cargo RPM macros a few days ago. Sorry about that! > I pushed a followup fix to dist-git for that, the package now builds thanks > Looks like some APIs are not available on those two architectures? > If you don't need rutabaga-gfx-ffi on ppc64le and s390x, I would just use > "ExclusiveArch: x86_64 aarch64". Yes, bindgen doesn't seem to support them. I reported the issue to some of the maintainers (I am not sure how to report bugs upstream..) I'll go with ExclusiveArch for now. thanks
FEDORA-2023-c58da38579 has been submitted as an update to Fedora 40. https://bodhi.fedoraproject.org/updates/FEDORA-2023-c58da38579
FEDORA-2023-c58da38579 has been pushed to the Fedora 40 stable repository. If problem still persists, please make note of it in this bug report.