Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-0.1-1.20220906gitc8f5ed9.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones
This is not quite ready for review yet! I have some questions of the package author to be resolved first. Stefan: - Shouldn't this depend on liburing-devel? I added that as a BR, but I'm not sure if that is correct. - Cargo doesn't build the *.so.* files, so I had to manually link them which does not seem ideal: pushd %{buildroot}%{_libdir} ln -s libblkio.so libblkio.so.0 ln -s libblkio.so.0 libblkio.so.0.0 popd - I suspect this will not actually build in Koji because it's going to try to pull in a whole load of cargo dependencies which are not listed as BRs. Will try a scratch build soon and post the results.
As expected the build attempts to download stuff, and that doesn't end well: https://koji.fedoraproject.org/koji/taskinfo?taskID=91712209
librsvg2 is a C lib with parts rust
In librsvg2: $ du -sh vendor/ 278M vendor/ After running 'cargo vendor' in libblkio sources: $ du -sh vendor/ 123M vendor/ Wow.
There are broadly two ways to build rust packages such as this: 1) Package all the rust crates that it needs in Fedora instead of downloading from the internet. 2) Use vendoring (another word for bundling in rust land) to include all the required rust crates in the tarball. Right now we are using option 2 in librsvg2, but over the last few days I've been trying to get the missing rust crates that it requires (there are hundreds of crates required for building librsvg2, but less than 10 are missing in Fedora right now) packaged up in Fedora proper. It's actually surprising simple to package a crate: we have a generator (rust2rpm) that spits out a ready spec file to go and it's so good that it usually doesn't need any fixes at all afterwards. So coming from the perspective of someone who has historically much preferred option 2, I think option 1 is actually very viable these days because it's so easy to package rust dependencies and because we already have a large amount of them pre-packaged. There's a rust BuildRequires generator that makes all the dependency generation automatic. See %generate_buildrequires section (currently %if'd out) in librsvg2 spec file for details :) https://src.fedoraproject.org/rpms/librsvg2/blob/rawhide/f/librsvg2.spec
The ones needed for libblkio are, with "*" meaning NOT present in Fedora, "?" meaning unsure: autocfg bitflags cc ? cfg-if * concat-idents const-cstr * io-uring lazy_static libc * memfd memmap memoffset nix proc-macro2 quote ? syn ? unicode-xid virtio-bindings * winapi * winapi-i686-pc-windows-gnu * winapi-x86_64-pc-windows-gnu
https://lists.fedoraproject.org/archives/list/devel@lists.fedoraproject.org/thread/BIDRSE72WRGNCB7BY3JJFDWQ5UQZG53K/ Looks like we should use cargo-c to fix the missing symlinks.
(In reply to Richard W.M. Jones from comment #6) > The ones needed for libblkio are, with "*" meaning NOT present in Fedora, > "?" meaning unsure: What does "unsure" mean in this context? > ? cfg-if https://src.fedoraproject.org/rpms/rust-cfg-if > * io-uring I tried to package this a few months ago (for tokio-uring and actix-rt), but frequent breakage of io_uring kernel APIs made it very painful. I can try again to see if this has improved. > * memfd Was packaged but retired: https://src.fedoraproject.org/rpms/rust-memfd > ? syn https://src.fedoraproject.org/rpms/rust-syn > ? unicode-xid https://src.fedoraproject.org/rpms/rust-unicode-xid > * winapi > * winapi-i686-pc-windows-gnu > * winapi-x86_64-pc-windows-gnu These three *MUST NOT BE PACKAGED*, they are bindings for Windows APIs.
> What does "unsure" mean in this context? Just meant I had problems finding them initially in Fedora, but it turns out they are in fact all present. I've just uploaded my latest attempt at packaging (which still doesn't work) so hit Shift + Reload on the spec file.
I can package the three remaining missing crates, if that helps you (concat-idents, io-uring, memfd).
This is a bit better: https://koji.fedoraproject.org/koji/taskinfo?taskID=91736622 I'll chase upstream about the i686 failure (if we care about that). This is done by "vendoring" the 6 missing dependencies: cat >> Cargo.toml <<EOF [patch.crates-io] concat-idents = { path = 'vendor/concat-idents' } io-uring = { path = 'vendor/io-uring' } memfd = { path = 'vendor/memfd' } winapi = { path = 'vendor/winapi' } winapi-i686-pc-windows-gnu = { path = 'vendor/winapi-i686-pc-windows-gnu' } winapi-x86_64-pc-windows-gnu = { path = 'vendor/winapi-x86_64-pc-windows-gnu' } EOF %cargo_prep sed -e 's/--locked/--offline/' -i src/cargo-build.sh Hopefully the winapi ones are not actually used, but they appear to be necessary. The other 3 should likely be added to Fedora.
(In reply to Fabio Valentini from comment #10) > I can package the three remaining missing crates, if that helps you > (concat-idents, io-uring, memfd). Yes that would help. Cc me on the package reviews if you need a reviewer.
Hi Rich, Thank you for this work. libblkio 1.0 has now been released: https://gitlab.com/libblkio/libblkio/-/tree/v1.0.0 The C API is now stable. The licenses have been changed : 1. libblkio - MIT OR Apache-2.0 2. blkio - MIT OR Apache-2.0 3. virtio-driver - (MIT OR Apache-2.0) AND BSD-3-Clause
(In reply to Richard W.M. Jones from comment #1) > Stefan: > > - Shouldn't this depend on liburing-devel? I added that > as a BR, but I'm not sure if that is correct. No. The io-uring crate directly invokes the io_uring system calls instead of going through the liburing library. > - Cargo doesn't build the *.so.* files, so I had to manually link them > which does not seem ideal: > > pushd %{buildroot}%{_libdir} > ln -s libblkio.so libblkio.so.0 > ln -s libblkio.so.0 libblkio.so.0.0 > popd I'm not sure about cargo's capabilities for sonames. libblkio currently doesn't use any special versioning configuration (e.g. symbol versioning, explicitly setting soname, etc).
> I'll chase upstream about the i686 failure (if we care about that). Alberto fixed the i686 build. libblkio 1.0.0 should build successfully now.
Just a couple more clarifications needed: - Should cargo-c be used? Apparently (I have not checked) it supports creating .so symlinks and handling .pc files. - Will libblkio & blkio live in the same upstream repository or be split out upstream? I think this may have implications for how we package in Fedora, although TBH either should be workable.
(In reply to Richard W.M. Jones from comment #16) > - Will libblkio & blkio live in the same upstream repository or be > split out upstream? I think this may have implications for how we > package in Fedora, although TBH either should be workable. & same question about virtio-driver
(In reply to Richard W.M. Jones from comment #16) > Just a couple more clarifications needed: > > - Should cargo-c be used? Apparently (I have not checked) it supports > creating .so symlinks and handling .pc files. libblkio currently uses meson for the library installation and pkg-config file. It does not create .so symlinks. I'm open to changing the build, if necessary. What would you recommend? > - Will libblkio & blkio live in the same upstream repository or be > split out upstream? I think this may have implications for how we > package in Fedora, although TBH either should be workable. For the time being they live in the same git repo but blkio is published on crates.io while libblkio is not (it's a C library, not a Rust crate).
It was really me pointing out that possibility, what you do is for upstream to decide :-) I think my question is are we going to go ahead with this package right now (with bundling), OR shall we wait for the dependencies to be built, OR shall we wait until you split upstream into blkio + libblkio (which would mean in turn that we're really doing two packages here and will need an extra review)? Let me know how you want to proceed on this one.
(In reply to Richard W.M. Jones from comment #19) > It was really me pointing out that possibility, what you do is for upstream > to decide :-) > > I think my question is are we going to go ahead with this package right now > (with bundling), OR shall we wait for the dependencies to be built, OR shall > we wait until you split upstream into blkio + libblkio (which would mean > in turn that we're really doing two packages here and will need an extra > review)? > Let me know how you want to proceed on this one. The blkio and virtio-driver crates are being developed in libblkio.git for the time being. Changing that is not a priority, so we can work with the current layout. Bundling blkio and virtio-driver is fine by me. At some point another Rust program may be packaged by Fedora that also uses blkio or virtio-driver. At that point unbundling would be natural. Right now the only program I'm aware of that uses blkio and virtio-driver is qsd-rs (https://gitlab.com/hreitz/qsd-rs), an experimental codebase that isn't close to be packaged.
Hi Rich, libblkio 1.1 has been released: https://gitlab.com/libblkio/libblkio/-/commits/v1.1.0/ It adds a virtio-blk-vfio-pci driver.
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.0-1.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93464214 I made several changes: - Update libblkio to version 1.1.0 - New bundled deps: * vendor/num-traits * vendor/pci-driver - Add "Provides: bundled(rust-...)" for bundled deps. - Remove hand-generated libblkio.so.* symlinks.
I wonder why you're bundling num-traits? It's up-to-date in Fedora. I also started to look at packaging the missing dependencies, and it looks like the dependency on memfd is quite old (v0.4), upstream is at v0.7 now - is there a reason for that? If I go through package review for memfd again, I'd rather have it not be for a version that can't even be used for libblkio.
> - Add "Provides: bundled(rust-...)" for bundled deps. The correct syntax for this would be "Provides: bundled(crate($foo)) = $version".
(In reply to Fabio Valentini from comment #23) > I wonder why you're bundling num-traits? It's up-to-date in Fedora. I must have missed it when I was looking at the new dependencies. I'll do an updated package that fixes this & the other things. > I also started to look at packaging the missing dependencies, and it looks > like the dependency on memfd is quite old (v0.4), upstream is at v0.7 now - > is there a reason for that? If I go through package review for memfd again, > I'd rather have it not be for a version that can't even be used for libblkio. Stefan? ^
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.0-2.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93518039
Review request for the io-uring crate (with some changes to ensure it works correctly on all architectures): https://bugzilla.redhat.com/show_bug.cgi?id=2138378 Can you check whether the packages which are still missing are: - concat-idents - memfd (v0.4 or latest v0.7?) - pci-driver
Yes, just those 3 left once io-uring gets in. I don't know the answer to the question about memfd version, hopefully Stefan will reply to the needinfo with that information.
Stefan, as well as the memfd version question, same question about io-uring 0.5.6 vs 0.5.8.
Review requests: - rust-concat-idents: https://bugzilla.redhat.com/show_bug.cgi?id=2138409 - rust-pci-driver: https://bugzilla.redhat.com/show_bug.cgi?id=2138410 I'll wait with the re-review / unretirement of rust-memfd until I know which version we need.
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.0-3.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Updated package: - I patched blkio to use io-uring 0.5.8 - I removed the bundled concat-idents & pci-driver & io-uring The only bundled packages now are memfd & the winapi-* stuff. Weirdly this builds fine in Koji but *not* locally (fails with a strange io-uring error which I don't understand). Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93622342
I wonder what still pulls in the winapi crates? memfd doesn't pull them in, and what we have packaged for Fedora has the Windows-specific dependencies stripped from it. > Weirdly this builds fine in Koji but *not* locally (fails with a strange > io-uring error which I don't understand). Huh, koji / local environments are not 100% identical - when I encountered similar problems, either seccomp filtering or slightly different network configuration was to blame. And since you mention io_uring, I kind of assume the former. BTW, do you have a reviewer for this package yet, or do you want me to take a look?
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.0-4.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones This one completely removes winapi* bundles. As it turns out those dependencies were not needed after all. Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=93625623 Here's the io-uring local build failure: http://oirase.annexia.org/reviews/libblkio/io-uring-fail.txt (Note that I'm patching libblkio to change the io-uring version. The failure is related to this patch. This does not happen with the upstream version / io-uring 0.5.6) Yes, it'd be great if you could review this!
> This one completely removes winapi* bundles. As it turns out those > dependencies were not needed after all. Great! Yeah, I assumed that they were leftovers, probably they had been pulled in by one of the other, previously vendored dependencies. > Here's the io-uring local build failure: > http://oirase.annexia.org/reviews/libblkio/io-uring-fail.txt That's interesting. Looks like the io_uring struct definitions in the kernel headers have changed *again* ... Is the version of the kernel-headers package in koji different than in your local build environment? > Yes, it'd be great if you could review this! I'll get to it, though I can't promise whether I'll have time for the first pass today or tomorrow.
Oh I didn't think this would be related to C / kernel headers, but yes you're right. I previously had kernel-headers-5.19.0-0.rc3.git0.1.fc37.x86_64, but upgrading to 6.1.0-0.rc2.git0.1.fc38 fixes the local build! Which is annoying .. I hope we don't have to keep chasing endless kernel changes. But here we are.
Also means backporting this package to RHEL is going to be painful, but luckily that's somebody else's problem.
Hi Fabio, Apologies for the late reply. memfd can be updated though the dependencies have grown in recent memfd releases due to the switch from the libc crate to rustix (see Cargo.lock diff below). I have confirmed that libblkio builds and passes tests with memfd 0.6.1. I've sent a libblkio pull request to update the memfd version here: https://gitlab.com/libblkio/libblkio/-/merge_requests/143 diff --git a/Cargo.lock b/Cargo.lock index 222c7d7..c20375a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -57,6 +57,33 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ed3d0b5ff30645a68f35ece8cea4556ca14ef8a1651455f789a099a0513532a6" +[[package]] +name = "errno" +version = "0.2.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f639046355ee4f37944e44f60642c6f3a7efa3cf6b78c78a0d989a8ce6c396a1" +dependencies = [ + "errno-dragonfly", + "libc", + "winapi", +] + +[[package]] +name = "errno-dragonfly" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "aa68f1b12764fab894d2755d2518754e71b4fd80ecfb822714a1206c2aab39bf" +dependencies = [ + "cc", + "libc", +] + +[[package]] +name = "io-lifetimes" +version = "0.7.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ce5ef949d49ee85593fc4d3f3f95ad61657076395cbbce23e2121fc5542074" + [[package]] name = "io-uring" version = "0.5.6" @@ -85,17 +112,23 @@ dependencies = [ [[package]] name = "libc" -version = "0.2.132" +version = "0.2.137" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8371e4e5341c3a96db127eb2465ac681ced4c433e01dd0e938adbef26ba93ba5" +checksum = "fc7fcc620a3bff7cdd7a365be3376c97191aeaccc2a603e600951e452615bf89" + +[[package]] +name = "linux-raw-sys" +version = "0.0.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d4d2456c373231a208ad294c33dc5bff30051eafd954cd4caae83a712b12854d" [[package]] name = "memfd" -version = "0.4.1" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6627dc657574b49d6ad27105ed671822be56e0d2547d413bfbf3e8d8fa92e7a" +checksum = "480b5a5de855d11ff13195950bdc8b98b5e942ef47afc447f6615cdcc4e15d80" dependencies = [ - "libc", + "rustix", ] [[package]] @@ -166,6 +199,20 @@ dependencies = [ "proc-macro2", ] +[[package]] +name = "rustix" +version = "0.35.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "727a1a6d65f786ec22df8a81ca3121107f235970dc1705ed681d3e6e8b9cd5f9" +dependencies = [ + "bitflags", + "errno", + "io-lifetimes", + "libc", + "linux-raw-sys", + "windows-sys", +] + [[package]] name = "syn" version = "1.0.99" @@ -223,3 +270,60 @@ name = "winapi-x86_64-pc-windows-gnu" version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "712e227841d057c1ee1cd2fb22fa7e5a5461ae8e48fa2ca79ec42cfc1931183f" + +[[package]] +name = "windows-sys" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5a3e1820f08b8513f676f7ab6c1f99ff312fb97b553d30ff4dd86f9f15728aa7" +dependencies = [ + "windows_aarch64_gnullvm", + "windows_aarch64_msvc", + "windows_i686_gnu", + "windows_i686_msvc", + "windows_x86_64_gnu", + "windows_x86_64_gnullvm", + "windows_x86_64_msvc", +] + +[[package]] +name = "windows_aarch64_gnullvm" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41d2aa71f6f0cbe00ae5167d90ef3cfe66527d6f613ca78ac8024c3ccab9a19e" + +[[package]] +name = "windows_aarch64_msvc" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dd0f252f5a35cac83d6311b2e795981f5ee6e67eb1f9a7f64eb4500fbc4dcdb4" + +[[package]] +name = "windows_i686_gnu" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fbeae19f6716841636c28d695375df17562ca208b2b7d0dc47635a50ae6c5de7" + +[[package]] +name = "windows_i686_msvc" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "84c12f65daa39dd2babe6e442988fc329d6243fdce47d7d2d155b8d874862246" + +[[package]] +name = "windows_x86_64_gnu" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf7b1b21b5362cbc318f686150e5bcea75ecedc74dd157d874d754a2ca44b0ed" + +[[package]] +name = "windows_x86_64_gnullvm" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09d525d2ba30eeb3297665bd434a54297e4170c7f1a44cad4ef58095b4cd2028" + +[[package]] +name = "windows_x86_64_msvc" +version = "0.42.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f40009d85759725a34da6d89a94e63d7bdc50a862acf0dbc7c8e488f1edcb6f5"
Looks like we have: - https://src.fedoraproject.org/rpms/rust-errno - https://src.fedoraproject.org/rpms/rust-io-lifetimes - https://src.fedoraproject.org/rpms/rust-linux-raw-sys - https://src.fedoraproject.org/rpms/rust-rustix and we are missing: - errno-dragonfly (I ignored the windows ones)
(and obviously memfd still too)
Great, thanks for confirming that memfd 0.6 is fine as well. I've submitted a re-review request for it here: https://bugzilla.redhat.com/show_bug.cgi?id=2140674 We already have rustix and all its dependencies packaged and up-to-date in Fedora (except for things that are blocked by the F37 Freeze). So there should be no missing dependencies other than memfd (errno-dragonfly is a dependency that is specific to, uhm, well, DragonflyBSD, and not relevant to us).
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.0-5.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Removed the memfd dependency (see bug 2140674). No scratch build because that requires memfd to appear in Fedora first. This contains two downstream patches to adjust the version numbers of dependencies. Hopefully the need for these should disappear: http://oirase.annexia.org/reviews/libblkio/0001-Use-io-uring-0.5.8.patch http://oirase.annexia.org/reviews/libblkio/0002-virtio-driver-upgrade-to-memfd-0.6.1.patch
Well ... https://gitlab.com/libblkio/libblkio/-/commit/500b4fdc9b95f8e75f2a98bb1861e2a35b0f5972 drops the dependency on memfd! Apparently nix provides access to equivalent functionality. Dependencies upstream now are: autocfg (ok) bitflags (ok) cc (ok) cfg-if (ok) concat-idents (ok) const-cstr (ok) io-uring (ok - also updated to 0.5.8) lazy_static (ok) libc (ok) memmap2 (ok - NEW) nix (ok) num-traits (ok) pci-driver (ok) proc-macro2 (ok) quote (ok) syn (ok) unicode-ident (ok) virtio-bindings (ok) Removed: memfd memmap memoffset winapi* Since there's quite a large change here I'm going to wait until upstream tag a new release.
Hi Rich, The libblkio 1.1.1 release is now available without the memfd crate dependency: https://gitlab.com/libblkio/libblkio/-/tree/v1.1.1
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.1-6.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build FAILS for unclear reasons. It builds fine locally. https://koji.fedoraproject.org/koji/taskinfo?taskID=94425117 Update to 1.1.1 - Remove downstream patches. - Remove memfd, memmap, memoffset dependencies. - Add memmap2 dependency. - Use SPDX code for license. - Remove all remaining bundles.
It builds fine against the f37 target: https://koji.fedoraproject.org/koji/taskinfo?taskID=94425359
meson 0.64.0 added a new optimization level called "plain" that means "don't set explicit compiler optimization options" (apparently nix needs this because it controls the compiler options). libblkio's src/cargo-build.sh script parses the meson_optimization option and doesn't know how to deal with "plain". I am investigating further to see how to solve this.
(In reply to Richard W.M. Jones from comment #44) > Scratch build FAILS for unclear reasons. It builds fine locally. > https://koji.fedoraproject.org/koji/taskinfo?taskID=94425117 Looks like a newer libc declaring read() and write() with __attribute__((warn_unused_result)). This should fix it: https://gitlab.com/libblkio/libblkio/-/merge_requests/148
(In reply to Alberto Faria from comment #47) > (In reply to Richard W.M. Jones from comment #44) > > Scratch build FAILS for unclear reasons. It builds fine locally. > > https://koji.fedoraproject.org/koji/taskinfo?taskID=94425117 > > Looks like a newer libc declaring read() and write() with > __attribute__((warn_unused_result)). This should fix it: > https://gitlab.com/libblkio/libblkio/-/merge_requests/148 Nevermind, we aren't using -Werror.
Hi Rich, Is it possible to add the following to the rpm spec file? export CARGO_NET_OFFLINE=true # avoid the need to patch src/cargo-build.sh # Explicitly enable debuginfo and release build optimization level for now. # In the future this can be dropped when rpm macros set RUSTFLAGS # themselves like they set CFLAGS, CXXFLAGS, etc. {%meson} -Ddebug=true -Doptimization=3 I think this will work on both f37 and f38. For f38 please see this libblkio merge request: https://gitlab.com/libblkio/libblkio/-/merge_requests/149
(In reply to Stefan Hajnoczi from comment #49) > Hi Rich, > Is it possible to add the following to the rpm spec file? > > export CARGO_NET_OFFLINE=true # avoid the need to patch src/cargo-build.sh The above failed with the error: error: the lock file /home/rjones/rpmbuild/BUILD/libblkio-v1.1.1/Cargo.lock needs to be updated but --locked was passed to prevent this (The original code which edited cargo-build.sh was designed to prevent that) > # Explicitly enable debuginfo and release build optimization level for now. > # In the future this can be dropped when rpm macros set RUSTFLAGS > # themselves like they set CFLAGS, CXXFLAGS, etc. > {%meson} -Ddebug=true -Doptimization=3 Making just this change, here are new scratch builds: f37: https://koji.fedoraproject.org/koji/taskinfo?taskID=94429101 f38: https://koji.fedoraproject.org/koji/taskinfo?taskID=94429097 > I think this will work on both f37 and f38. > > For f38 please see this libblkio merge request: > https://gitlab.com/libblkio/libblkio/-/merge_requests/149
Both builds whinged about: + /usr/bin/meson --buildtype=plain --prefix=/usr --libdir=/usr/lib64 --libexecdir=/usr/libexec --bindir=/usr/bin --sbindir=/usr/sbin --includedir=/usr/include --datadir=/usr/share --mandir=/usr/share/man --infodir=/usr/share/info --localedir=/usr/share/locale --sysconfdir=/etc --localstatedir=/var --sharedstatedir=/var/lib --wrap-mode=nodownload --auto-features=enabled . redhat-linux-build -Ddebug=true -Doptimization=3 WARNING: Recommend using either -Dbuildtype or -Doptimization + -Ddebug. Using both is redundant since they override each other. See: https://mesonbuild.com/Builtin-options.html#build-type-options
(In reply to Richard W.M. Jones from comment #51) > Both builds whinged about: > > + /usr/bin/meson --buildtype=plain --prefix=/usr --libdir=/usr/lib64 > --libexecdir=/usr/libexec --bindir=/usr/bin --sbindir=/usr/sbin > --includedir=/usr/include --datadir=/usr/share --mandir=/usr/share/man > --infodir=/usr/share/info --localedir=/usr/share/locale --sysconfdir=/etc > --localstatedir=/var --sharedstatedir=/var/lib --wrap-mode=nodownload > --auto-features=enabled . redhat-linux-build -Ddebug=true -Doptimization=3 > WARNING: Recommend using either -Dbuildtype or -Doptimization + -Ddebug. > Using both is redundant since they override each other. See: > https://mesonbuild.com/Builtin-options.html#build-type-options It's a warning, not an error. The build should still proceed. I think we have to accept the warning for now, unless we're willing to switch away from the %meson macro entirely.
(In reply to Richard W.M. Jones from comment #50) > (In reply to Stefan Hajnoczi from comment #49) > > Hi Rich, > > Is it possible to add the following to the rpm spec file? > > > > export CARGO_NET_OFFLINE=true # avoid the need to patch src/cargo-build.sh > > The above failed with the error: > > error: the lock file /home/rjones/rpmbuild/BUILD/libblkio-v1.1.1/Cargo.lock > needs to be updated but --locked was passed to prevent this > > (The original code which edited cargo-build.sh was designed to prevent that) > > > # Explicitly enable debuginfo and release build optimization level for now. > > # In the future this can be dropped when rpm macros set RUSTFLAGS > > # themselves like they set CFLAGS, CXXFLAGS, etc. > > {%meson} -Ddebug=true -Doptimization=3 > > Making just this change, here are new scratch builds: > > f37: https://koji.fedoraproject.org/koji/taskinfo?taskID=94429101 > f38: https://koji.fedoraproject.org/koji/taskinfo?taskID=94429097 Cool, the builds are succeeding!
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.1-7.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch builds: f37 https://koji.fedoraproject.org/koji/taskinfo?taskID=94452911 f38 https://koji.fedoraproject.org/koji/taskinfo?taskID=94452912 I think we may be ready for review now.
Note that using BuildRequires on Rust crates like this: BuildRequires: crate(cc/default) Might or might not do what you want it to do, because you didn't include version requirements. Not specifying a version will always resolve to the *latest* version, which might not be the one you need. Specifying the acceptable version range like this BuildRequires: (crate(cc/default) >= 1.0.44 with crate(cc/default) < 2.0.0~) (or whatever minimum version of the crate is specified) is much safer, and will always result in the correct version of the crate getting installed.
I could certainly encode the information from the package into the spec file, but this seems to duplicate the information. Do we have any way to keep the two in synch automatically? Stefan ^^ do you have any preference here?
> Do we have any way to keep the two in synch automatically? Yeah, that's what dynamic BuildRequires are for. The macro that does that for cargo doesn't support workspaces / path dependencies yet, though :( But until I have time to implement and test that, you can use a workaround like this one here: https://src.fedoraproject.org/rpms/system76-keyboard-configurator/blob/rawhide/f/system76-keyboard-configurator.spec#_36-45
(In reply to Richard W.M. Jones from comment #56) > I could certainly encode the information from the package into the > spec file, but this seems to duplicate the information. Do we have > any way to keep the two in synch automatically? > > Stefan ^^ do you have any preference here? Preventing the major version number from changing sounds good. Other than that, libblkio doesn't have specific version requirements for cc/default. Some of the crate dependencies have pre-1.0 version numbers where breaking API changes can occur in any new version (even minor or patch versions, according to semver). In those cases it would be best to hardcode a specific package version? Either way, it may be necessary to update the spec file as new crate versions are packaged for Fedora :/.
No, cc was just an example, I was making a general statement about all Rust dependencies that are listed as BuildRequires. The automatically generated BuildRequires already include version specifiers that are compatible with Cargo metadata. Additionally, Cargo-SemVer actually provides stronger stability guarantees than plain SemVer - for example, 0.6.x and 0.6.y should be API-compatible with each other. So specifying *exact* version requirements (i.e. something like "= 0.6.5") actually a very bad idea (and unnecessary).
(In reply to Fabio Valentini from comment #55) > BuildRequires: (crate(cc/default) >= 1.0.44 with crate(cc/default) < > 2.0.0~) Does ~ here have a meaning or was it a typo?
It's not a typo, it's necessary to prevent the expression from matching 2.0.0 pre-releases. Using just "< 2.0.0" would include versions like 2.0.0~beta.1, which is not what "^1.0.44" means.
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.1-8.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=94485089 I didn't fully understand the script posted in comment 57, so I just added some boolean version dependencies. For dependencies with major version >= 1, I added the requirement that the major version doesn't change. For major version < 1, I added a simple minimum requirement. We would want to switch to using %generate_buildrequires at some time in the future. Also unicode-xid -> unicode-ident (this changed in an earlier version but we didn't notice).
Why didn't you do the same for pre-1.0 versions? The same restrictions apply there. For example, the "nix" crate will likely be updated to version 0.25.0 soon, and using the >= 0.24.2 requirement you have now, doing that would break this package (as it would start to pull in rust-nix-0.25, making the build of this package fail with missing dependency on nix v0.24). Instead, use "(crate(nix/default) >= 0.24.2 with crate(nix/default) < 0.25~)" so that this package will continue pulling in the nix v0.24 compat package once the main nix package is updated to v0.25. As I mentioned, cargo-semver provides stronger compatibility guarantees than plain semver in this regard (for pre-1.0-versions), as patch releases in the 0.24.x series are supposed to be compatible with each other - but minor releases of pre-1.0 versions are treated just like "major" version changes (i.e. something like v0.24 -> v0.25). The only range of versions where "exact" requirements are fine are for pre-0.1-versions like "0.0.x", where a change from "0.0.x" to "0.0.x+1" conveys a breaking change, as well (if you discount pre-releases).
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.1-9.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=94487848 Enforce same minor version for 0.x dependencies.
Hi Fabio, Is there anything I can do to help this make progress? Thanks!
Hi! Sorry for the radio silence. I didn't remember that I had assigned this bug to myself ... I don't really have much time to spend on Fedora these days, so I cannot promise a formal review of this package. If somebody else wants to take a stab at it before I can come back to this, feel free to un-assign me from this bug and take over. For what it's worth, I think the Rust related things in this package look good now, with one exception: The way cargo is invoked in src/cargo-build.sh is not compatible with Fedora packaging guidelines, as it makes cargo ignore the default compiler flags in Fedora, which is explicitly forbidden. So if at all possible, the cargo-build.sh script should probably not be used during the build, but it should be possible to replicate the only two lines in it that matter, i.e. """ ( set -x && RUSTFLAGS="${rustflags[*]}" cargo build "${args[@]}" --color always \ --locked ) cp "src/target/${profile}/libblkio.so" "src/libblkio.so.${version}" """ with calls to the %cargo_build macro (making the sed "s/--locked/--offline/g" also unnecessary) and the cp call in %build. Not sure how that change could be integrated in meson, though - but does it even need to be? I.e. would it be possible to make %build something like """ %build %meson %cargo_build cp "src/target/${profile}/libblkio.so" "src/libblkio.so.${version}" %meson_build """ And patch meson.build to not run cargo-build.sh at all?
Thanks, Fabio! The rpm could invoke cargo directly, although the build won't be 100% the same as tested upstream. I think it's preferable to use meson instead of bypassing it. I think the build is almost correct already because rpm invokes meson with buildtype "plain". That tells meson not to set optimization flags and cargo-build.sh honors that (see case "${meson_optimization}"). They are inherited from the environment. However, cargo-build.sh currently overrides RUSTFLAGS. Instead, it should use RUSTFLAGS="$RUSTFLAGS ${rustflags[*]}". That way rpm can pass any options via RUSTFLAGS while non-rpm builds can still set compiler flags in cargo-build.sh. How does that sound?
The problem is that RUSTFLAGS are not set automatically, and are not exposed directly, but passed as environment variables to the cargo command in the %cargo_build macro. So there's currently no way to "just set RUSTFLAGS", as that's not really a supported use case right now. So even if your meson setup doesn't override the flags, there's nothing to inherit from the environment - because they are not *set* in the environment.
It might be possible to partially work around this by writing the value of the `%build_rustflags` macro to the RUSTFLAGS environment variable, with something like: export RUSTFLAGS="%build_rustflags" (untested) And looking at the difference between the values of "%cargo_build" "/usr/bin/env CARGO_HOME=.cargo RUSTC_BOOTSTRAP=1 RUSTFLAGS='-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now --cap-lints=warn' /usr/bin/cargo build -j16 -Z avoid-dev-deps --release" and "%build_rustflags" "-Copt-level=3 -Cdebuginfo=2 -Ccodegen-units=1 -Clink-arg=-Wl,-z,relro -Clink-arg=-Wl,-z,now --cap-lints=warn" that *should* be enough, since the script is already passing `--release`. We'll still need to verify that the flags actually end up being passed through correctly cargo / rustc - especially `-Cdebuginfo=2` - otherwise builds of the package won't get valid debug information.
Thanks for the %build_rustflags suggestion! My cargo-build.sh patch has been merged upstream, so it's now possible to pass RUSTFLAGS. Rich: Is it possible to incorporate "export RUSTFLAGS="%build_rustflags"" as suggested by Fabio into the spec file and include the following libblkio patch? https://gitlab.com/libblkio/libblkio/-/commit/44652d6c7fe9c61ac417db9dd05ec27e3c6e20f9.patch
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.1.1-10.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=95326385 I wasn't very clear on exactly where I should add the new export, so I guessed.
error: failed to select a version for the requirement `io-uring = ">=0.5.6, <=0.5.8"` candidate versions found which didn't match: 0.5.9 This build failure seems to be a problem with libblkio. I don't see any upstream patch to handle the new version of io-uring in Fedora.
> ">=0.5.6, <=0.5.8" Hum, that's kind of a very weird version restriction. I updated io-uring to version 0.5.9 yesterday, so that explains why things are broken. Usually you'd want to do just "0.5.6" or "^0.5.6", which are both equivalent to ">= 0.5.6, < 0.6.0". I freely update Rust crates in Fedora from 0.x.y to 0.x.y+1, because these are always supposed to be backwards compatible. Unusual / non-standard restrictions like this one are very annoying to deal with, as they kind of circumvent guarantees of SemVer.
> I wasn't very clear on exactly where I should add the new export, > so I guessed. Yeah, adding it in %build before calling %meson and %meson_build should do it. I'll verify that the build flags are applied correctly once it builds again.
> sed -e 's/--locked/--offline/' -i src/cargo-build.sh I'm also pretty sure you should be able to just drop "--locked" and not add "--offline", as that flag should be a no-op assuming the cargo configuration is injected correctly by %cargo_prep.
(In reply to Richard W.M. Jones from comment #72) > error: failed to select a version for the requirement `io-uring = ">=0.5.6, > <=0.5.8"` > candidate versions found which didn't match: 0.5.9 > > This build failure seems to be a problem with libblkio. I don't > see any upstream patch to handle the new version of io-uring in > Fedora. Upstream libblkio is trying to stay compatible with stable Debian, which is still on Rust 1.48, but io-uring 0.5.9 requires Rust 1.51. I sent https://github.com/tokio-rs/io-uring/pull/161 to fix that, but we'll have to wait for io-uring 0.5.10 to be released and packaged, then update libblkio's blkio/Cargo.toml to allow io-uring 0.5.10. (In reply to Fabio Valentini from comment #73) > > ">=0.5.6, <=0.5.8" > > Hum, that's kind of a very weird version restriction. I updated io-uring to > version 0.5.9 yesterday, so that explains why things are broken. > > Usually you'd want to do just "0.5.6" or "^0.5.6", which are both equivalent > to ">= 0.5.6, < 0.6.0". > I freely update Rust crates in Fedora from 0.x.y to 0.x.y+1, because these > are always supposed to be backwards compatible. > Unusual / non-standard restrictions like this one are very annoying to deal > with, as they kind of circumvent guarantees of SemVer. Unfortunately we can't depend on io-uring "0.5" since we rely on unstable features.
> Upstream libblkio is trying to stay compatible with stable Debian, which is still on Rust 1.48, but io-uring 0.5.9 requires Rust 1.51. I don't see how this affects the range of supported versions? Assuming debian has packaged io-uring 0.5.8, and Fedora has packaged io-uring 0.5.9, both are compatible with something like ">=0.5.6, <0.5.10", unless you lock the version explicitly to either 0.5.8 or 0.5.9? But the Fedora build already drops the "--locked" flag for cargo, so that doesn't apply here, I think. > Unfortunately we can't depend on io-uring "0.5" since we rely on unstable features. That's unfortunate, and will be very annoying to deal with. I guess I'll have to put io-uring on a list of "special" crates that need to be handled differently from everything else (and it would be the only crate on that list, out of over 2000 that we have packaged for Fedora ...)
(In reply to Fabio Valentini from comment #77) > > Upstream libblkio is trying to stay compatible with stable Debian, which is still on Rust 1.48, but io-uring 0.5.9 requires Rust 1.51. > > I don't see how this affects the range of supported versions? Assuming > debian has packaged io-uring 0.5.8, and Fedora has packaged io-uring 0.5.9, > both are compatible with something like ">=0.5.6, <0.5.10", unless you lock > the version explicitly to either 0.5.8 or 0.5.9? But the Fedora build > already drops the "--locked" flag for cargo, so that doesn't apply here, I > think. More precisely, we're keeping the crates.io-based build of libblkio working on stable Debian, so users can install it from source. In fact, there doesn't seem to be a Debian package for the io-uring crate. > > Unfortunately we can't depend on io-uring "0.5" since we rely on unstable features. > > That's unfortunate, and will be very annoying to deal with. I guess I'll > have to put io-uring on a list of "special" crates that need to be handled > differently from everything else (and it would be the only crate on that > list, out of over 2000 that we have packaged for Fedora ...) I sent https://github.com/tokio-rs/io-uring/pull/162 proposing the removal of the "unstable" feature. It does seem redundant to have such a feature on a pre-1.0 crate.
io-uring 0.5.10 has been released. It restores compatibility with Rust 1.48 and also makes the entire public API follow the usual SemVer rules, making the "unstable" feature have no effect. libblkio 1.2.0 will be released today, adding some new functionality and updating the io-uring dependency version specifier to "0.5.10".
libblkio 1.2.0 has been released. It has the updated io-uring crate dependency.
Great, thanks for submitting that upstream. I've already updated the rust-io-uring package in Fedora to 0.5.10.
(In reply to Fabio Valentini from comment #75) > > sed -e 's/--locked/--offline/' -i src/cargo-build.sh > > I'm also pretty sure you should be able to just drop "--locked" and not add > "--offline", as that flag should be a no-op assuming the cargo > configuration is injected correctly by %cargo_prep. If I remove this line then I get: error: the lock file /home/rjones/rpmbuild/BUILD/libblkio-v1.2.1/Cargo.lock needs to be updated but --locked was passed to prevent this
Oh I see, you said to drop --locked, but not add --offline. That works too ...
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.2.1-11.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=95585044 Version 1.2.1 - Remove patches which are all upstream. - Update io-uring dependency minimum to 0.5.10. - Don't add --offline to cargo-build.sh
libblkio v1.2.2 has been released. It only fixes a version number inconsistency, so there's no need to upgrade unless you noticed you're getting 1.2.0 version numbers instead of 1.2.1.
Hi Fabio, do you still want to take this package for review, or shall I look for someone else to review it? Fine for me either way.
Sorry, I didn't know whether you wanted to update to 1.2.2 before the review, and then dropped the ball over the holidays. I don't really have the bandwidth to deal with this right now, so I'm fine with you finding somebody else to finish the review. All I can say is that that Rust side of things appears to be looking good now, but I haven't looked at any other aspect of this package yet.
Taking this review.
Reviewing the spec, I see only one issue left: > %{_libdir}/libblkio.so.1* This is unnecessarily permissive. Change this to "%{_libdir}/libblkio.so.1{,.*}" so that we ensure that we're talking about "libblkio.so.1", rather than potentially "libblkio.so.10" or some other crazy permutation. Cf. https://docs.fedoraproject.org/en-US/packaging-guidelines/#_listing_shared_library_files
Spec URL: http://oirase.annexia.org/reviews/libblkio/libblkio.spec SRPM URL: http://oirase.annexia.org/reviews/libblkio/libblkio-1.2.2-12.fc37.src.rpm Description: Block device I/O library Fedora Account System Username: rjones Scratch build: https://koji.fedoraproject.org/koji/taskinfo?taskID=96003601 I fixed the %files problem with the library. I suspect this affects other packages I manage, never really thought about it before. I also updated to the new upstream version 1.2.2.
Argh, the %changelog version is wrong - will fix in my local copy.
Copr build: https://copr.fedorainfracloud.org/coprs/build/5216289 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2124697-libblkio/fedora-rawhide-x86_64/05216289-libblkio/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
> [!]: Reviewer should test that the package builds in mock. - I scratch-built the package in Koji which should cover this. > libblkio.x86_64: W: incoherent-version-in-changelog 1.2.1-12 ['1.2.2-12.fc38', '1.2.2-12'] > 4 packages and 0 specfiles checked; 0 errors, 1 warnings, 0 badness; has taken 0.3 s This is true - fixed locally.
Review notes: - Package is named appropriately - Package (mostly) following Rust packaging guidelines - Package builds and installs - No serious issues from rpmlint PACKAGE APPROVED.
https://pagure.io/releng/fedora-scm-requests/issue/50385
The Pagure repository was created at https://src.fedoraproject.org/rpms/libblkio
https://pagure.io/releng/fedora-scm-requests/issue/50386 (separate request for f37 branch)
FEDORA-2023-a4a9a1ca5c has been submitted as an update to Fedora 37. https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4a9a1ca5c
FEDORA-2023-a4a9a1ca5c has been pushed to the Fedora 37 testing repository. Soon you'll be able to install the update with the following command: `sudo dnf install --enablerepo=updates-testing --refresh --advisory=FEDORA-2023-a4a9a1ca5c \*` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2023-a4a9a1ca5c See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.
FEDORA-2023-a4a9a1ca5c has been pushed to the Fedora 37 stable repository. If problem still persists, please make note of it in this bug report.