Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.2%5egit20231027.d320a4e-1.fc39.src.rpm Description: The bcachefs-tools package provides all the userspace programs needed to create, check, modify and correct any inconsistencies in the bcachefs filesystem. Fedora Account System Username: ngompa
FYI, this does not yet build properly. I am working on getting it into shape.
Copr build: https://copr.fedorainfracloud.org/coprs/build/6586498 (failed) Build log: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06586498-bcachefs-tools/builder-live.log.gz Please make sure the package builds successfully at least for Fedora Rawhide. - If the build failed for unrelated reasons (e.g. temporary network unavailability), please ignore it. - If the build failed because of missing BuildRequires, please make sure they are listed in the "Depends On" field --- 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.
FYI: I was able to build a rpm using the spec file included in the git repo itself, ref. https://evilpiepirate.org/git/bcachefs-tools.git/tree/packaging/bcachefs-tools.spec
(In reply to Tony Asleson from comment #3) > FYI: I was able to build a rpm using the spec file included in the git repo > itself, ref. > https://evilpiepirate.org/git/bcachefs-tools.git/tree/packaging/bcachefs- > tools.spec There are two problems: * There's some kind of forked-ish Rust dependencies that look like they can be unforked, so that the dependency generator would work. * The fuse code FTBFS: gcc -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -std=gnu11 -O2 -g -MMD -Wall -fPIC -Wno-pointer-sign -Wno-deprecated-declarations -fno-strict-aliasing -fno-delete-null-pointer-checks -I. -Iinclude -Iraid -D_FILE_OFFSET_BITS=64 -D_GNU_SOURCE -D_LGPL_SOURCE -DRCU_MEMBARRIER -DZSTD_STATIC_LINKING_ONLY -DFUSE_USE_VERSION=32 -DNO_BCACHEFS_CHARDEV -DNO_BCACHEFS_FS -DNO_BCACHEFS_SYSFS -DVERSION_STRING='"1.3.1"' -Wno-unused-but-set-variable -Wno-stringop-overflow -Wno-zero-length-bounds -Wno-missing-braces -Wno-shift-overflow -Wno-enum-conversion -DBCACHEFS_FUSE -I/usr/include/uuid -I/usr/include/blkid -I/usr/include/fuse3 -DBCACHEFS_NO_RUST -c -o cmd_fusemount.o cmd_fusemount.c cmd_fusemount.c:24:10: fatal error: libbcachefs/io.h: No such file or directory 24 | #include "libbcachefs/io.h" | ^~~~~~~~~~~~~~~~~~ compilation terminated. make: *** [Makefile:113: cmd_fusemount.o] Error 1
Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.1-1.fc39.src.rpm This updates to 1.3.1 and turns off both fuse and rust for the moment. It builds, but I'm not happy with it yet.
The forked rust-bindgen at https://evilpiepirate.org/git/rust-bindgen.git has to do with allowing packed+aligned structures, which aren't allowed by default. I was able to work around this by creating a git-archive of the above and adding it as Source1, then modifying rust-src/bch_bindgen/Cargo.toml to refer to the local copy, i.e. sed -i -e 's#git = "https://evilpiepirate.org/git/rust-bindgen.git"#path = "PATH/TO/rust-bindgen-modified/bindgen"#' rust-src/bch_bindgen/Cargo.toml or similar. Probably breaks all sorts of packaging rules but unless Kent publishes his modified rust-bindgen on crates.io or something (which seems a little sketch as well) I'm not sure there's another way around it.
(In reply to Tony Asleson from comment #3) > FYI: I was able to build a rpm using the spec file included in the git repo > itself, ref. > https://evilpiepirate.org/git/bcachefs-tools.git/tree/packaging/bcachefs-tools.spec Yep that should work fine, I had sent a patch to do a maybe-to-clever rpm build within the source tree. And for local builds, there's no problem w/ grabbing the modified rust-bindgen via git ... it gets much more complicated on the build hosts though, I think.
Also, from Kent: "fuse is incomplete and broken, don't need that"
I figured that much out when the fuse stuff didn't compile. :)
Created attachment 1997177 [details] The .spec file difference from Copr build 6586498 to 6599195
Copr build: https://copr.fedorainfracloud.org/coprs/build/6599195 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06599195-bcachefs-tools/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.
I talked w/ Kent about this and I think the quickest path forward to getting a working build is for him to "vendor" the dependencies in his release tarballs. That's been done at https://evilpiepirate.org/bcachefs-tools/bcachefs-tools-vendored-1.3.2.tar.zst This seems to work in general (tho this is with my spec file I think): https://koji.fedoraproject.org/koji/taskinfo?taskID=108731223 i686 failed but excluding that arch for now is probably reasonable.
Taking this review
Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.2-1.fc39.src.rpm This has several updates to make it work to comply with Fedora packaging requirements as much as I can...
Created attachment 1997779 [details] The .spec file difference from Copr build 6599195 to 6611419
Copr build: https://copr.fedorainfracloud.org/coprs/build/6611419 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06611419-bcachefs-tools/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.
Package Review ============== Legend: [x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated [ ] = Manual review needed Issues: ======= - The License field must be a valid SPDX expression. Note: Not a valid SPDX expression 'GPL-2.0-only and (Apache-2.0 and (Apache-2.0 or MIT) and (Apache-2.0 with LLVM-exception or Apache-2.0 or MIT) and MIT and MPL-2.0 and (Unlicense or MIT))'. See: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 ===== MUST items ===== C/C++: [x]: Provides: bundled(gnulib) in place as required. Note: Sources not installed [x]: Package does not contain kernel modules. [x]: If your application is a C or C++ application you must list a BuildRequires against gcc, gcc-c++ or clang. [x]: Header files in -devel subpackage, if present. [x]: Package does not contain any libtool archives (.la) [x]: Package contains no static executables. [x]: Rpath absent or only used for internal libs. Generic: [x]: Package is licensed with an open-source compatible license and meets other legal requirements as defined in the legal section of Packaging Guidelines. [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License, Version 2", "*No copyright* GNU General Public License, Version 2", "GNU General Public License v2.0 or later and/or PostgreSQL License", "GNU Lesser General Public License, Version 2.1", "BSD 3-Clause License", "GNU General Public License v2.0 or later", "BSD 2-Clause License and/or GNU General Public License, Version 2", "*No copyright* GNU Lesser General Public License, Version 2.1", "*No copyright* Creative Commons CC0 1.0", "GNU General Public License", "Public domain", "*No copyright* The Unlicense", "MIT License", "*No copyright* MIT License", "*No copyright* Apache License 2.0", "Apache License 2.0 and/or MIT License", "*No copyright* Mozilla Public License 2.0", "*No copyright* Apache License (v2.0) or MIT license", "Apache License 2.0 and/or BSD 3-Clause License", "Apache License 2.0", "Unicode License Agreement - Data Files and Software (2016)", "Apache License (v2.0) or MIT license", "Apache License (v2.0) or MIT license and/or MIT License", "*No copyright* Apache License (v2.0) or MIT license and/or MIT License". 6315 files have unknown license. Detailed output of licensecheck in /tmp/2247350-bcachefs-tools/licensecheck.txt [x]: License file installed when any subpackage combination is installed. [x]: If the package is under multiple licenses, the licensing breakdown must be documented in the spec. [x]: %build honors applicable compiler flags or justifies otherwise. [!]: Package contains no bundled libraries without FPC exception. [x]: Changelog in prescribed format. [x]: Sources contain only permissible code or content. [-]: Package contains desktop file if it is a GUI application. [x]: Development files must be in a -devel package [x]: Package uses nothing in %doc for runtime. [x]: Package consistently uses macros (instead of hard-coded directory names). [x]: Package is named according to the Package Naming Guidelines. [x]: Package does not generate any conflict. [x]: Package obeys FHS, except libexecdir and /usr/target. [-]: If the package is a rename of another package, proper Obsoletes and Provides are present. [x]: Requires correct, justified where necessary. [x]: Spec file is legible and written in American English. [-]: Package contains systemd file(s) if in need. [x]: Useful -debuginfo package or justification otherwise. [x]: Package is not known to require an ExcludeArch tag. [x]: Package complies to the Packaging Guidelines [x]: Package successfully compiles and builds into binary rpms on at least one supported primary architecture. [x]: Package installs properly. [x]: Rpmlint is run on all rpms the build produces. Note: There are rpmlint messages (see attachment). [x]: If (and only if) the source package includes the text of the license(s) in its own file, then that file, containing the text of the license(s) for the package is included in %license. [x]: Package requires other packages for directories it uses. [x]: Package must own all directories that it creates. [x]: Package does not own files or directories owned by other packages. [x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT [x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the beginning of %install. [x]: Macros in Summary, %description expandable at SRPM build time. [x]: Dist tag is present. [x]: Package does not contain duplicates in %files. [x]: Permissions on files are set properly. [x]: Package must not depend on deprecated() packages. [x]: Package use %makeinstall only when make install DESTDIR=... doesn't work. [x]: Package is named using only allowed ASCII characters. [x]: Package does not use a name that already exists. [x]: Package is not relocatable. [x]: Sources used to build the package match the upstream source, as provided in the spec URL. [x]: Spec file name must match the spec package %{name}, in the format %{name}.spec. [x]: File names are valid UTF-8. [x]: Large documentation must go in a -doc subpackage. Large could be size (~1MB) or number of files. Note: Documentation size is 0 bytes in 0 files. [x]: Packages must not store files under /srv, /opt or /usr/local ===== SHOULD items ===== Generic: [-]: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [x]: Final provides and requires are sane (see attachments). [x]: Package functions as described. [x]: Latest version is packaged. [x]: Package does not include license text files separate from upstream. [x]: Patches link to upstream bugs/comments/lists or are otherwise justified. [-]: Sources are verified with gpgverify first in %prep if upstream publishes signatures. Note: gpgverify is not used. [x]: Package should compile and build into binary rpms on all supported architectures. [-]: %check is present and all tests pass. [x]: Packages should try to preserve timestamps of original installed files. [x]: Reviewer should test that the package builds in mock. [x]: Buildroot is not present [x]: Package has no %clean section with rm -rf %{buildroot} (or $RPM_BUILD_ROOT) [x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin. [x]: Fully versioned dependency in subpackages if applicable. [x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file [x]: Sources can be downloaded from URI in Source: tag [x]: SourceX is a working URL. [x]: Spec use %global instead of %define unless justified. ===== EXTRA items ===== Generic: [x]: Rpmlint is run on debuginfo package(s). Note: No rpmlint messages. [x]: Rpmlint is run on all installed packages. Note: There are rpmlint messages (see attachment). [x]: Large data in /usr/share should live in a noarch subpackage if package is arched. [x]: Spec file according to URL is the same as in SRPM. Rpmlint ------- Checking: bcachefs-tools-1.3.2-1.fc40.aarch64.rpm bcachefs-tools-debuginfo-1.3.2-1.fc40.aarch64.rpm bcachefs-tools-debugsource-1.3.2-1.fc40.aarch64.rpm bcachefs-tools-1.3.2-1.fc40.src.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpu0xd31wp')] checks: 31, packages: 4 bcachefs-tools.aarch64: W: no-manual-page-for-binary fsck.bcachefs bcachefs-tools.aarch64: W: no-manual-page-for-binary mkfs.bcachefs bcachefs-tools.aarch64: W: no-manual-page-for-binary mount.bcachefs bcachefs-tools.src: W: inconsistent-file-extension bcachefs-tools-vendored-1.3.2.tar.zst 4 packages and 0 specfiles checked; 0 errors, 4 warnings, 0 badness; has taken 1.0 s Rpmlint (debuginfo) ------------------- Checking: bcachefs-tools-debuginfo-1.3.2-1.fc40.aarch64.rpm ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml rpmlintrc: [PosixPath('/tmp/tmpc74a32og')] checks: 31, packages: 1 1 packages and 0 specfiles checked; 0 errors, 0 warnings, 0 badness; has taken 0.3 s Rpmlint (installed packages) ---------------------------- ============================ rpmlint session starts ============================ rpmlint: 2.4.0 configuration: /usr/lib/python3.12/site-packages/rpmlint/configdefaults.toml /etc/xdg/rpmlint/fedora-legacy-licenses.toml /etc/xdg/rpmlint/fedora-spdx-licenses.toml /etc/xdg/rpmlint/fedora.toml /etc/xdg/rpmlint/scoring.toml /etc/xdg/rpmlint/users-groups.toml /etc/xdg/rpmlint/warn-on-functions.toml checks: 31, packages: 3 bcachefs-tools.aarch64: W: no-manual-page-for-binary fsck.bcachefs bcachefs-tools.aarch64: W: no-manual-page-for-binary mkfs.bcachefs bcachefs-tools.aarch64: W: no-manual-page-for-binary mount.bcachefs 3 packages and 0 specfiles checked; 0 errors, 3 warnings, 0 badness; has taken 0.7 s Source checksums ---------------- https://evilpiepirate.org/bcachefs-tools/bcachefs-tools-vendored-1.3.2.tar.zst : CHECKSUM(SHA256) this package : 4c0db6557da2cd37f75844252a787b813f0cbf96e006230d60c7a200bcdbdc11 CHECKSUM(SHA256) upstream package : 4c0db6557da2cd37f75844252a787b813f0cbf96e006230d60c7a200bcdbdc11 Requires -------- bcachefs-tools (rpmlib, GLIBC filtered): ld-linux-aarch64.so.1()(64bit) libaio.so.1()(64bit) libaio.so.1(LIBAIO_0.1)(64bit) libaio.so.1(LIBAIO_0.4)(64bit) libblkid.so.1()(64bit) libblkid.so.1(BLKID_2.15)(64bit) libblkid.so.1(BLKID_2.17)(64bit) libblkid.so.1(BLKID_2.21)(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) libkeyutils.so.1()(64bit) libkeyutils.so.1(KEYUTILS_0.3)(64bit) liblz4.so.1()(64bit) libsodium.so.26()(64bit) libudev.so.1()(64bit) libudev.so.1(LIBUDEV_183)(64bit) liburcu-common.so.8()(64bit) liburcu.so.8()(64bit) libuuid.so.1()(64bit) libuuid.so.1(UUID_1.0)(64bit) libz.so.1()(64bit) libzstd.so.1()(64bit) rtld(GNU_HASH) bcachefs-tools-debuginfo (rpmlib, GLIBC filtered): bcachefs-tools-debugsource (rpmlib, GLIBC filtered): Provides -------- bcachefs-tools: bcachefs-tools bcachefs-tools(aarch-64) bundled(crate(aho-corasick)) bundled(crate(anstream)) bundled(crate(anstyle)) bundled(crate(anstyle-parse)) bundled(crate(anstyle-query)) bundled(crate(anstyle-wincon)) bundled(crate(anyhow)) bundled(crate(atty)) bundled(crate(autocfg)) bundled(crate(bindgen)) bundled(crate(bitfield)) bundled(crate(bitflags)) bundled(crate(byteorder)) bundled(crate(cc)) bundled(crate(cexpr)) bundled(crate(cfg-if)) bundled(crate(chrono)) bundled(crate(clang-sys)) bundled(crate(clap)) bundled(crate(clap_builder)) bundled(crate(clap_derive)) bundled(crate(clap_lex)) bundled(crate(colorchoice)) bundled(crate(colored)) bundled(crate(either)) bundled(crate(errno)) bundled(crate(errno-dragonfly)) bundled(crate(fastrand)) bundled(crate(filedescriptor)) bundled(crate(gag)) bundled(crate(getset)) bundled(crate(glob)) bundled(crate(heck)) bundled(crate(hermit-abi)) bundled(crate(io-lifetimes)) bundled(crate(is-terminal)) bundled(crate(itertools)) bundled(crate(lazy_static)) bundled(crate(lazycell)) bundled(crate(libc)) bundled(crate(libudev-sys)) bundled(crate(linux-raw-sys)) bundled(crate(log)) bundled(crate(memchr)) bundled(crate(memoffset)) bundled(crate(minimal-lexical)) bundled(crate(nom)) bundled(crate(num-traits)) bundled(crate(once_cell)) bundled(crate(parse-display)) bundled(crate(parse-display-derive)) bundled(crate(paste)) bundled(crate(peeking_take_while)) bundled(crate(pkg-config)) bundled(crate(proc-macro-error)) bundled(crate(proc-macro-error-attr)) bundled(crate(proc-macro2)) bundled(crate(quote)) bundled(crate(redox_syscall)) bundled(crate(regex)) bundled(crate(regex-automata)) bundled(crate(regex-syntax)) bundled(crate(rpassword)) bundled(crate(rustc-hash)) bundled(crate(rustix)) bundled(crate(shlex)) bundled(crate(strsim)) bundled(crate(syn)) bundled(crate(tempfile)) bundled(crate(terminal_size)) bundled(crate(thiserror)) bundled(crate(thiserror-impl)) bundled(crate(udev)) bundled(crate(unicode-ident)) bundled(crate(utf8parse)) bundled(crate(uuid)) bundled(crate(version_check)) bundled(crate(winapi)) bundled(crate(winapi-i686-pc-windows-gnu)) bundled(crate(winapi-x86_64-pc-windows-gnu)) bundled(crate(windows-sys)) bundled(crate(windows-targets)) bundled(crate(windows_aarch64_gnullvm)) bundled(crate(windows_aarch64_msvc)) bundled(crate(windows_i686_gnu)) bundled(crate(windows_i686_msvc)) bundled(crate(windows_x86_64_gnu)) bundled(crate(windows_x86_64_gnullvm)) bundled(crate(windows_x86_64_msvc)) bcachefs-tools-debuginfo: bcachefs-tools-debuginfo bcachefs-tools-debuginfo(aarch-64) debuginfo(build-id) bcachefs-tools-debugsource: bcachefs-tools-debugsource bcachefs-tools-debugsource(aarch-64) Generated by fedora-review 0.10.0 (e79b66b) last change: 2023-07-24 Command line :/usr/bin/fedora-review -b 2247350 Buildroot used: fedora-rawhide-aarch64 Active plugins: Shell-api, Generic, C/C++ Disabled plugins: Haskell, Java, Python, Perl, SugarActivity, Ocaml, PHP, fonts, R Disabled flags: EXARCH, EPEL6, EPEL7, DISTTAG, BATCH
- The License field must be a valid SPDX expression. Note: Not a valid SPDX expression 'GPL-2.0-only and (Apache-2.0 and (Apache-2.0 or MIT) and (Apache-2.0 with LLVM-exception or Apache-2.0 or MIT) and MIT and MPL-2.0 and (Unlicense or MIT))'. See: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 I think SPDX wants AND and OR to be capitalized [!]: License field in the package spec file matches the actual license. Note: Checking patched sources after %prep for licenses. Licenses found: "Unknown or generated", "GNU General Public License, Version 2", "*No copyright* GNU General Public License, Version 2", "GNU General Public License v2.0 or later and/or PostgreSQL License", "GNU Lesser General Public License, Version 2.1", "BSD 3-Clause License", "GNU General Public License v2.0 or later", "BSD 2-Clause License and/or GNU General Public License, Version 2", "*No copyright* GNU Lesser General Public License, Version 2.1", "*No copyright* Creative Commons CC0 1.0", "GNU General Public License", "Public domain", "*No copyright* The Unlicense", "MIT License", "*No copyright* MIT License", "*No copyright* Apache License 2.0", "Apache License 2.0 and/or MIT License", "*No copyright* Mozilla Public License 2.0", "*No copyright* Apache License (v2.0) or MIT license", "Apache License 2.0 and/or BSD 3-Clause License", "Apache License 2.0", "Unicode License Agreement - Data Files and Software (2016)", "Apache License (v2.0) or MIT license", "Apache License (v2.0) or MIT license and/or MIT License", "*No copyright* Apache License (v2.0) or MIT license and/or MIT License". 6315 files have unknown license. Detailed output of licensecheck in /tmp/2247350-bcachefs-tools/licensecheck.txt You'll want to recheck the license breakdown, there's definitely stuff you're not currently listing (e.g. some BSD-3-Clause under libbcachefs/siphash.{c,h}). You also need to add bundled Provides for all the vendored stuff (and ensure that's factored into the license breakdown as well). And ideally we should engage with upstream to get rid of the vendored stuff altogether in the long run, as this won't be terribly maintainable as-is.
Some more notes: - there's a manpage and an "operating manual" under doc/ you should get included in the package - there are tests provided under tests/ that we should at least attempt to run in %check - the initramfs hook will need to be converted to a dracut config (and ideally upstreamed)
Per Kent, the tests in tests/ are not useful. Designed for the fuse stuff, and "should probably be deleted"
The version at https://evilpiepirate.org/bcachefs-tools/bcachefs-tools-vendored-1.3.3.tar.zst now seems to build properly on all arches (the i686 build is fixed)
Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.3-1.fc39.src.rpm This updates to 1.3.3 and attempts to address the other feedback.
Created attachment 1997969 [details] The .spec file difference from Copr build 6611419 to 6614237
Copr build: https://copr.fedorainfracloud.org/coprs/build/6614237 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06614237-bcachefs-tools/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.
I'm not going to be de-vendoring the dependencies; sorry, but that would be adding far too much maintenance burden on myself. The license breakdown is something we can look at; this is something cargo really ought to be able to help us out with - there should be a standard mechanism for that for those dependencies. Is there an established process yet?
(In reply to Kent Overstreet from comment #25) > I'm not going to be de-vendoring the dependencies; sorry, but that would be > adding far too much maintenance burden on myself. > I'm confused by this statement. Maintaining a fork of bindgen should be a bigger maintenance burden than using published crates. I don't think it's unreasonable to ask for you to at least try to contribute your changes so that you don't have forked crates anymore. I see no attempt to do so on https://github.com/rust-lang/rust-bindgen/pulls?q=is%3Apr+from%3Akoverstreet
I think there are 2 issues here related to the vendoring business. (with the caveat that I'm a complete rust n00b, and know very little about how Fedora is approaching it.) The first issue is the forked bindgen; for now, there is no way around that I think. Packaging the forked version w/ bcachefs-tools seems to be the straightforward solution for now. The second issue is all the other crate dependencies. I *think* Fedora prefers to have those crates packaged as separate RPMs, and in fact some of them (or all of them? I haven't looked) already are. I was not able to sort out how to get the bcachefs-tools RPM build to use them, but somebody in Fedora-land must know. Kent has expressed displeasure at the idea of using separately packaged crates for the build, but the truth is if it's an OS packaging policy, there's not a lot upstream can do about that. (Conversation and input, sure, but in the end, the OS will have a policy that upstream may or may not agree with.)
The forked bindgen is going to be a necessity for a good while; the patch that bcachefs requires is a hack that would not be acceptable to upstream, and a proper fix is going to require digging into rustc. That will happen, but it'll probably be ~six months out. And it's only a build dependency, not a runtime dependency. If Fedora is going to attempt to be turning every cargo dependency into a separate RPM package - it's open source, you are of course free too if you want, but that won't be an upstream supported approach.
I've just tagged 1.3.4, which includes COPYING.rust-dependencies.
Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.4-1.fc39.src.rpm This updates to 1.3.4 and attempts to address the other feedback.
(In reply to Kent Overstreet from comment #28) > The forked bindgen is going to be a necessity for a good while; the patch > that bcachefs requires is a hack that would not be acceptable to upstream, > and a proper fix is going to require digging into rustc. > Then at least file an issue with upstream so it's tracked. I see nothing from you there, which prevents *anyone* from doing anything about it: https://github.com/rust-lang/rust-bindgen/issues?q=author%3Akoverstreet
Created attachment 2000066 [details] The .spec file difference from Copr build 6614237 to 6663352
Copr build: https://copr.fedorainfracloud.org/coprs/build/6663352 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06663352-bcachefs-tools/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.
Here, I've opened one: https://github.com/rust-lang/rust/issues/118018
So in conversation with Eric a few other things came up - LTO builds, for one. Besides swapping out/devendoring dependencies, I need to ask that packagers be restrained in what else they change about how bcachefs-tools is built - if there's something you'd like to change/enable, like LTO, please do it by submitting a patch to bcachefs-tools and not monkey-patching in the rpm build process. We need to make sure that people using bcachefs are using packages that have been properly tested; that means running code that's built the same way as what my test infrastructure uses: https://evilpiepirate.org/~testdashboard/ci LTO is great, but it's not something that we can flip on without testing; there's particular cause for concern here because bcachefs-tools is a mixed C and Rust project, and by default links gcc and llvm output together. In kernel land this works - but not necessarily when you get into strange build configurations, of which LTO is one. Regarding use of dependencies from cargo vs. distro: again, I don't want users running something I haven't tested. Keep in mind that we're talking about filesystem tools here, so any mistake may result in users not being able to boot their machines. And I do mean any mistake; for example clap (which we use for argument parsing) may seem like something trivial that distro packagers can swap out for a different version without cause for concern - but if a change in argument parsing results in subtle breakage, then our mount helper may break and users won't get out of their initramfs. That won't be fun for the average user to recover from. I understand that distro people have concerns about proper vetting/auditing of packages from other package managers, and I'd like to see those taken up - by working with the Cargo people. I also need to note that Rust doesn't yet support dynamic linking across crates (with #repr(Rust)), so there's no advantage to de-vendoring packages as far as runtime overhead.
(In reply to Kent Overstreet from comment #35) For context: I am a member of the Fedora Packaging Committee, the Rust SIG (and the developer and maintainer of the current version of the Rust packaging tools), and of FESCo, but I'm not responding here with that hat on. To me, it looks like there are some underlying misunderstandings here about how Linux distributions "work" (Fedora, in this case) - I've tried to address some of your points from the previous comment below. > So in conversation with Eric a few other things came up - LTO builds, for > one. > > Besides swapping out/devendoring dependencies, I need to ask that packagers > be restrained in what else they change about how bcachefs-tools is built - > if there's something you'd like to change/enable, like LTO, please do it by > submitting a patch to bcachefs-tools and not monkey-patching in the rpm > build process. To the contrary - Fedora packages *MUST* be built with the default compiler flags set by the distribution - calling this "monkey-patching" is not accurate. Unless there are actual problems that are caused by some flags, just disabling them is not OK. > We need to make sure that people using bcachefs are using packages that have > been properly tested; that means running code that's built the same way as > what my test infrastructure uses: https://evilpiepirate.org/~testdashboard/ci I'm sorry, but I really don't understand this argument. I assume we don't even use the same C / Rust compilers (or the same versions) - I really hope you don't want us to ship and use the same version of the C / Rust compilers and the linker that you used to build and test, too? > LTO is great, but it's not something that we can flip on without testing; > there's particular cause for concern here because bcachefs-tools is a mixed > C and Rust project, and by default links gcc and llvm output together. In > kernel land this works - but not necessarily when you get into strange build > configurations, of which LTO is one. LTO is not a "strange build configuration". It's been enabled by default distro-wide in Fedora 33, about *three years ago*: https://fedoraproject.org/wiki/LTOByDefault Fedora is also not special here - some other distributions have defaulted to building with LTO enabled for even longer than that. > Regarding use of dependencies from cargo vs. distro: again, I don't want > users running something I haven't tested. This is also out of your hands, given how packaging works, be it Rust or C or whatever - you just *cannot* guarantee (or expect) that we will ship the exact versions of dependencies that you tested with - or the same version of the C / Rust compiler that you compiled your tests with, for what it's worth. Making sure that the packages that *we* ship actually work is also not *your* responsibility, but that of the *packager*. > Keep in mind that we're talking about filesystem tools here, so any mistake > may result in users not being able to boot their machines. And I do mean any > mistake; for example clap (which we use for argument parsing) may seem like > something trivial that distro packagers can swap out for a different version > without cause for concern - but if a change in argument parsing results in > subtle breakage, then our mount helper may break and users won't get out of > their initramfs. This is not the first project that's written in Rust that now makes up some kind of critical component in Fedora - even RPM itself now has dependencies that are written in Rust. The way we package components like these has never caused any "subtle" issues like the ones you're thinking of here. > That won't be fun for the average user to recover from. > > I understand that distro people have concerns about proper vetting/auditing > of packages from other package managers, and I'd like to see those taken up > - by working with the Cargo people. That's just asking us to reinvent the wheel. We *already have* a way to vet and audit projects from language-specific package ecosystems. ... by providing them as packages. > I also need to note that Rust doesn't yet support dynamic linking across > crates (with #repr(Rust)), so there's no advantage to de-vendoring packages > as far as runtime overhead. There is no advantage for "runtime overhead", but there are other advantages to packaging Rust crates individually (for example, the "vetting/auditing" thing). Also, all that is somewhat irrelevant to this review ticket - you are neither the person who submitted the package for review, nor the person who is doing the review. While upstream project developer's opinions can - and do - inform how we do *some* things, they cannot override rules that we have for good reasons.
> I'm sorry, but I really don't understand this argument. I assume we don't even use the same C / Rust compilers (or the same versions) - I really hope you don't want us to ship and use the same version of the C / Rust compilers and the linker that you used to build and test, too? No. What I want is, if you're going to enable LTO, do it by submitting a patch to bcachefs-tools so it can be properly tested. > To the contrary - Fedora packages *MUST* be built with the default compiler flags set by the distribution - calling this "monkey-patching" is not accurate. Unless there are actual problems that are caused by some flags, just disabling them is not OK. I seriously doubt you change the compiler flags for the kernel to something unsupported. > This is not the first project that's written in Rust that now makes up some kind of critical component in Fedora - even RPM itself now has dependencies that are written in Rust. The way we package components like these has never caused any "subtle" issues like the ones you're thinking of here. I'm sorry, but RPM won't cause the system to fail to boot, or corrupt user data, if it has bugs. Us filesystem people are _considerably_ touchier than most about having a proper QA process. > This is also out of your hands, given how packaging works, be it Rust or C or whatever - you just *cannot* guarantee (or expect) that we will ship the exact versions of dependencies that you tested with - or the same version of the C / Rust compiler that you compiled your tests with, for what it's worth. > Making sure that the packages that *we* ship actually work is also not *your* responsibility, but that of the *packager*. No, this attitude right here is a total non starter. I'm the one who's primarily going to be on the hook for bcachefs bug reports; I have not seen any of you involved in bcachefs, nor have I seen any kind of a QA process from you guys. Given that, this "we know best, and you will have no involvement" attitude isn't going to fly. I haven't gotten this working with any other distributions; NixOS, Debian and Arch people have been totally fine to work with, and have been in active communication when necessary and contribute changes back. Let's just put a pause on this whole process, shall we?
(In reply to Kent Overstreet from comment #34) > Here, I've opened one: https://github.com/rust-lang/rust/issues/118018 Thank you for that. (In reply to Kent Overstreet from comment #37) > > I'm sorry, but I really don't understand this argument. I assume we don't even use the same C / Rust compilers (or the same versions) - I really hope you don't want us to ship and use the same version of the C / Rust compilers and the linker that you used to build and test, too? > > No. What I want is, if you're going to enable LTO, do it by submitting a > patch to bcachefs-tools so it can be properly tested. > > > To the contrary - Fedora packages *MUST* be built with the default compiler flags set by the distribution - calling this "monkey-patching" is not accurate. > Unless there are actual problems that are caused by some flags, just > disabling them is not OK. > > I seriously doubt you change the compiler flags for the kernel to something > unsupported. > We do, in fact, build our kernel with our build flags. Some stuff *is* selectively disabled, but nearly all our build flags are used. While I see no evidence of LTO breaking bcachefs-tools, I can disable it for now until I've gotten more of a chance to poke at it and see that it's fine. > > This is not the first project that's written in Rust that now makes up some kind of critical component in Fedora - even RPM itself now has dependencies that are written in Rust. The way we package components like these has never caused any "subtle" issues like the ones you're thinking of here. > > I'm sorry, but RPM won't cause the system to fail to boot, or corrupt user > data, if it has bugs. > It definitely can do those things. And in the past *has* done some of those things. > Us filesystem people are _considerably_ touchier than most about having a > proper QA process. > I am aware of how filesystem people work. I have been working professionally with this stuff for almost a decade now with OpenZFS, Btrfs, and now bcachefs. I have been working with the Btrfs folks both upstream and in Fedora for the better part of that timeframe too. > > This is also out of your hands, given how packaging works, be it Rust or C or whatever - you just *cannot* guarantee (or expect) that we will ship the exact versions of dependencies that you tested with - or the same version of the C / Rust compiler that you compiled your tests with, for what it's worth. > > > Making sure that the packages that *we* ship actually work is also not *your* responsibility, but that of the *packager*. > > No, this attitude right here is a total non starter. > > I'm the one who's primarily going to be on the hook for bcachefs bug > reports; I have not seen any of you involved in bcachefs, nor have I seen > any kind of a QA process from you guys. > I don't get involved in anything unless I can ship it in Fedora. There is no point otherwise. Feel free to ask the Btrfs folks for my credentials, I'm very well-known there. > Given that, this "we know best, and you will have no involvement" attitude > isn't going to fly. I haven't gotten this working with any other > distributions; NixOS, Debian and Arch people have been totally fine to work > with, and have been in active communication when necessary and contribute > changes back. > You haven't looked at what Debian is doing, have you? Debian has disabled all the Rust parts because they haven't gotten through devendoring and packaging all the Rust crates you use, because Debian *mandates* it. They are also applying their distro-wide build flags too. I have chosen to use the vendored crates for now because it is not worth it to me while the forked crates situation exists. > Let's just put a pause on this whole process, shall we? No. This needs to land so we can turn on bcachefs in the Fedora kernel. At this point, the only thing left for me to do is to add GPG verification since it looks like you publish signatures for the tarballs. Unfortunately, I don't seem to be able to find the signing key, at least gpg can't download it. Could you publish it somewhere on your website? Here's a sample command for how it would work. gpg2 --export --export-options export-minimal 2A70052E44BC4216BE8EF42B13AB336D8DCA6E76 > gpgkey-2A70052E44BC4216BE8EF42B13AB336D8DCA6E76.gpg
Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.4-2.fc39.src.rpm This temporarily disables LTO, as requested by Kent.
Neal, I believe this is the signing key you can use: https://git.kernel.org/pub/scm/docs/kernel/pgpkeys.git/plain/keys/13AB336D8DCA6E76.asc
Yup, that worked. Thanks Eric. Spec URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools.spec SRPM URL: https://ngompa.fedorapeople.org/for-review/bcachefs-tools-1.3.4-3.fc39.src.rpm This now verifies the signatures. At this point, this is as good as it gets. Davide, can you please look over it now?
Created attachment 2000572 [details] The .spec file difference from Copr build 6663352 to 6671124
Copr build: https://copr.fedorainfracloud.org/coprs/build/6671124 (succeeded) Review template: https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2247350-bcachefs-tools/fedora-rawhide-x86_64/06671124-bcachefs-tools/fedora-review/review.txt Found issues: - Not a valid SPDX expression 'GPL-2.0-only and GPL-2.0-or-later and LGPL-2.1-only and BSD-3-Clause and (Apache-2.0 and (Apache-2.0 or MIT) and (Apache-2.0 with LLVM-exception or Apache-2.0 or MIT) and MIT and MPL-2.0 and (Unlicense or MIT))'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 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.
> - Not a valid SPDX expression 'GPL-2.0-only and GPL-2.0-or-later and LGPL-2.1-only and BSD-3-Clause and (Apache-2.0 and (Apache-2.0 or MIT) and (Apache-2.0 with LLVM-exception or Apache-2.0 or MIT) and MIT and MPL-2.0 and (Unlicense or MIT))'. Read more: https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1 This came up earlier, looks like the parser expects AND/OR to be capitalized per https://fedoraproject.org/wiki/Changes/SPDX_Licenses_Phase_1#With_Fedora_abbreviations_we_could_use_and,_or,_and_parentheses_when_constructing_expressions._Can_we_do_that_with_SPDX? > This temporarily disables LTO, as requested by Kent. Can you file a bugzilla once this is imported and/or an issue upstream to track this? > %doc doc/bcachefs.5.rst.tmpl This looks like a template for a manpage, but I don't see manpage itself getting included. All these are fixable on import, so it's APPROVED
> While I see no evidence of LTO breaking bcachefs-tools, I can disable it for now until I've gotten more of a chance to poke at it and see that it's fine. Thanks. Actually, a more likely source of breakage is any hardening flags you might be enabling; I was just spending a fair amount of time dealing with breakage from new kernel side hardening. Can we get a patch submitted to bcachefs-tools to add all those to the CFLAGS there? We definitely want LTO enabled, I just want testing and review for all of these. If you're flipping on hardening options, they should be in a warning mode, not "exit the process" mode; kernel side we've that e.g. the bounds checking that was added was triggering spuriously because we push VLAs pretty hard. > I am aware of how filesystem people work. I have been working professionally with this stuff for almost a decade now with OpenZFS, Btrfs, and now bcachefs. I have been working with the Btrfs folks both upstream and in Fedora for the better part of that timeframe too. Good to know. If you're going to be the main person involved on the Fedora side, I'd love to see you in the IRC channel - irc.oftc.net #bcache. > You haven't looked at what Debian is doing, have you? Debian has disabled all the Rust parts because they haven't gotten through devendoring and packaging all the Rust crates you use, because Debian *mandates* it. They are also applying their distro-wide build flags too. I have chosen to use the vendored crates for now because it is not worth it to me while the forked crates situation exists. No, I definitely have, and disabling the Rust part for now is a better solution in my eyes than devendoring. In recent years we've seen a lot of work on a lot of fronts on improving reproducibility of builds. What Cargo (and relatedly, NixOS) have been doing is a huge step in the right direction; a cargo lockfile now references _exact git sha1s_ of dependencies, which means that by checking in the lockfiles anyone building a particular revision of bcachefs-tools will get the same output, with respect to library dependencies. That's huge, considering that historically library version mismatch has been a huge source of build problems, in particular. (I was just fighting with this in someone else's C project, and as a result I can't reproduce the upstream smatch build and get the same source code analysis results. That sucks). It now means that we can catch all these sorts of issues with automated testing, before doing a release. I realize what we have now may not be what distribution people consider the ideal solution; it runs somewhat counter to your historical practices. But I consider it a big step forward, and I hope distribution people can do something that builds on top of what they achieved, and not take away the reproducibility of builds that's been achieved. The devendoring that's been talked about sounds too much like an experiment that I don't want to be the guinea pig for. > No. This needs to land so we can turn on bcachefs in the Fedora kernel. At this point, the only thing left for me to do is to add GPG verification since it looks like you publish signatures for the tarballs. There's no /need/ to do this on any particular schedule, and I would prefer to take things slow. I've spent 15 years on this project, taking my time to get things right, trying to produce something polished and stable. I don't want to burn my reputation by getting impatient and going too fast just as I'm at the finish line. I'm about to see a massive uptick in users and bug reports, just from bcachefs being merged into 6.7. It's going to be a lot to stay on top of, and dealing with bug reports in a timely manner is something I consider important: when I have the time to work with people it's an avenue to teaching people how to report bugs well, how to do useful testing, and it's how I've effectively built up a solid QA team. Not being able to respond to bug reports is a missed opportunity. So: a staged release, instead of everyone getting it all at once, is a _good_ thing. If the next 3 months after the release of 6.7 it's still primarily available to those building from source, I'm much more likely to be hearing from people I can work with.
The Pagure repository was created at https://src.fedoraproject.org/rpms/bcachefs-tools