https://bugzilla.redhat.com/show_bug.cgi?id=2274028 Fabio Valentini <decathorpe@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |decathorpe@xxxxxxxxx --- Comment #6 from Fabio Valentini <decathorpe@xxxxxxxxx> --- > I would like someone from RUST team to review as well, but here's my review: Ok, I see several issues with this package, some of which seem to have been added as workarounds to work around other issues ... > I think there's some packaging issue with devel packages for the backends. I see for example: > I would have expected different content there. Why? The "rust-%{crate}-devel" subpackage contains the source code for the crate. The "rust-%{crate}+$feature-devel subpackages are only present for metadata / dependency resolution and are empty. > rust-vhost-device-sound-devel (rpmlib, GLIBC filtered): > (crate(clap/default) >= 4.4.0 with crate(clap/default) < 5.0.0~) > (crate(clap/derive) >= 4.4.0 with crate(clap/derive) < 5.0.0~) > (crate(env_logger/default) >= 0.11.0 with crate(env_logger/default) < 0.12.0~) > (crate(log/default) >= 0.4.0 with crate(log/default) < 0.5.0~) > (crate(thiserror/default) >= 1.0.0 with crate(thiserror/default) < 2.0.0~) > (crate(vhost-user-backend/default) >= 0.13.0 with crate(vhost-user-backend/default) < 0.14.0~) > (crate(vhost/default) >= 0.10.0 with crate(vhost/default) < 0.11.0~) > (crate(vhost/vhost-user-backend) >= 0.10.0 with crate(vhost/vhost-user-backend) < 0.11.0~) > (crate(virtio-bindings/default) >= 0.2.1 with crate(virtio-bindings/default) < 0.3.0~) > (crate(virtio-queue/default) >= 0.11.0 with crate(virtio-queue/default) < 0.12.0~) > (crate(vm-memory/default) >= 0.14.0 with crate(vm-memory/default) < 0.15.0~) > (crate(vmm-sys-util/default) >= 0.12.0 with crate(vmm-sys-util/default) < 0.13.0~) > cargo > ld-linux-x86-64.so.2()(64bit) > libasound.so.2()(64bit) > libasound.so.2(ALSA_0.9)(64bit) > libasound.so.2(ALSA_0.9.0rc4)(64bit) > libc.so.6()(64bit) > libgcc_s.so.1()(64bit) > libgcc_s.so.1(GCC_3.0)(64bit) > libgcc_s.so.1(GCC_3.3)(64bit) > libgcc_s.so.1(GCC_4.2.0)(64bit) > libpipewire-0.3.so.0()(64bit) > rtld(GNU_HASH) This looks very weird. The "-devel" subpackage should NEVER be arched or depend on shared libraries. This might happen if the source code accidentally ships binaries (which it MUST NOT, it's supposed to be noarch). The -devel package is also missing README / LICENSE files. Not sure why they were removed from the generated spec file. > * The patch for `i386` should be called "build-fix-for-i686", since this is what we actually build in Fedora. > `i386` is only used in copr. The supported architectures in COPR and koji are the same, just the name is different (for whatever reason). > * Why do all packages have "BuildArch: noarch"? All -devel subpackages should have it, it's normal for Rust packages. They *are* architecture-independent, since these packages either ship source code (architecture-independent!) or are empty and only carry RPM metadata. > * The comment "# FIXME: paste output of %%cargo_license_summary here" should be removed since you wrote down the output of `%cargo_license_summary`. This is wrong, the output of %cargo_license_summary looks different, it's an itemized list. Right now the spec file just contains the same information twice. > fixed: warnings about Package contains duplicates in %files! This is not an issue, it's a useless warning provided by RPM and actually is a false positive. Now the files *are* missing %doc and %license tags. > I might misunderstand the purpose of the devel packages with rust language as I don't know the language at all. > But from a package named `rust-vhost-device-sound+pw-devel-0.1.0-1.fc41.x86_64.rpm` I would have expected content like: Yes, the "rust-$crate+$feature-devel" subpackages are empty and are only present for RPM metadata / dependency resolution. The "rust-$crate-devel" package contains all the actual source code. Splitting source code across multiple packages would break everything. > fixed: removed patches to ignore tests by skipping them in the `%check` This is not working as intended. Now you're just running tests 10 times and skipping *one* different test each time ... the list of skipped tests needs to be passed *to the same %cargo_test* invocation *once*. > the package dependencies specfiles specifies "BuildArch: noarch", > I think it'll be better to clarify with the maintainer of those packages. What do you mean here? The places where "rust2rpm" puts "BuildArch: noarch" should just not be changed at all. ================================================================================ Other issues: Applying a source code patch only on specific architectures will break things (as you have discovered). The source code MUST be architecture-independent. In this case, the code should be fixed to be architecture-agnostic. The fact that the code currently fails to compile on 32-bit architectures should be considered an upstream bug. The source of the issue seems to be that the functions in alsa take differently sized arguments on different architectures: hwp.set_period_size(i64::from(period_frames), alsa::ValueOr::Less) hwp.set_buffer_size_near(2 * i64::from(period_frames)) The type if the first argument is in each case a "Frames", which is defined as "alsa::snd_pcm_sframes_t", which is in turn defined as a C "long" in the alsa header files - which has a different size on different architectures. I would recommend to apply a patch like this instead (this should also be upstreamable): src/audio_backends/alsa.rs:163: - let period_frames = period_bytes / frame_size; + let period_frames = alsa::pcm::Frames::from(period_bytes / frame_size); src/audio_backends/alsa.rs:165: -hwp.set_period_size(i64::from(period_frames), alsa::ValueOr::Less)?; +hwp.set_period_size(period_frames, alsa::ValueOr::Less)?; src/audio_backends/alsa.rs:177: -if let Err(err) = hwp.set_buffer_size_near(2 * i64::from(period_frames)) { +if let Err(err) = hwp.set_buffer_size_near(2 * period_frames) { This patch can be applied on all architectures since it does the correct conversion regardless of target architecture. It makes the code both simpler *and* architecture-agnostic (*and* is simpler than the currently selectively applied patch) so I would recommend to send something like this upstream. At this point I would recommend starting fresh from a spec file generated by "rust2rpm" to drop all workarounds that were mistakenly added to work around *real issues*, and add the missing things from there. -- You are receiving this mail because: You are always notified about changes to this product and component You are on the CC list for the bug. https://bugzilla.redhat.com/show_bug.cgi?id=2274028 Report this comment as SPAM: https://bugzilla.redhat.com/enter_bug.cgi?product=Bugzilla&format=report-spam&short_desc=Report%20of%20Bug%202274028%23c6 -- _______________________________________________ package-review mailing list -- package-review@xxxxxxxxxxxxxxxxxxxxxxx To unsubscribe send an email to package-review-leave@xxxxxxxxxxxxxxxxxxxxxxx Fedora Code of Conduct: https://docs.fedoraproject.org/en-US/project/code-of-conduct/ List Guidelines: https://fedoraproject.org/wiki/Mailing_list_guidelines List Archives: https://lists.fedoraproject.org/archives/list/package-review@xxxxxxxxxxxxxxxxxxxxxxx Do not reply to spam, report it: https://pagure.io/fedora-infrastructure/new_issue
