Panic when checking bootloader crate

[phil-opp]

Steps to reproduce the bug with the above code

Run cargo semver-checks check-release on the https://github.com/rust-osdev/bootloader repository using cargo-semver-checks v0.18.1.

Actual Behaviour

cargo-semver-checks errors and creates a crash report:

name = 'cargo-semver-checks'
operating_system = 'Pop (22.04) (64-bit)'
crate_version = '0.18.1'
explanation = '''
Panic occurred in file '/.../.cargo/registry/src/github.com-1ecc6299db9ec823/cargo-semver-checks-0.18.1/src/manifest.rs' at line 14
'''
cause = 'called `Result::unwrap()` on an `Err` value: WorkspaceIntegrity("not all fields of `bootloader-boot-config` have been present in workspace.package")'
method = 'Panic'
backtrace = '''

   0: 0x55a5573bfd43 - core::result::unwrap_failed::h3ab6b432ea8a4ede
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/core/src/result.rs:1750
   1: 0x55a5574e5c7f - cargo_semver_checks::manifest::Manifest::parse::hcbe0ea02f9453a2a
   2: 0x55a55756c0d5 - cargo_semver_checks::rustdoc_gen::RustdocFromProjectRoot::new::h53c5d45983cd2717
   3: 0x55a5574937d6 - <core::iter::adapters::map::Map<I,F> as core::iter::traits::iterator::Iterator>::fold::hd76ece15a9dfc5d7
   4: 0x55a557520cdd - <alloc::vec::Vec<T> as alloc::vec::spec_from_iter::SpecFromIter<T,I>>::from_iter::h071eb1912ee603f1
   5: 0x55a55759772a - cargo_semver_checks::main::h3251edf4bbe6368c
   6: 0x55a557498fe3 - std::sys_common::backtrace::__rust_begin_short_backtrace::ha462ffb6999f62b8
   7: 0x55a5575b83ed - std::rt::lang_start::{{closure}}::h339d46bcdb9a48f7
   8: 0x55a557e1173c - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::h5be7dd97304b75d5
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/core/src/ops/function.rs:287
                 - std::panicking::try::do_call::h22610b2d36113d30
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panicking.rs:483
                 - std::panicking::try::hbf4a9dad9baa84d6
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panicking.rs:447
                 - std::panic::catch_unwind::h1e0170bbae1b4db9
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panic.rs:140
                 - std::rt::lang_start_internal::{{closure}}::hca1fa9d7cba9167a
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/rt.rs:148
                 - std::panicking::try::do_call::h4578381a4fe90710
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panicking.rs:483
                 - std::panicking::try::h7c6aea5570ec2a5a
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panicking.rs:447
                 - std::panic::catch_unwind::hb594e2e3afa0ae84
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/panic.rs:140
                 - std::rt::lang_start_internal::hade69f851696d91a
                at /rustc/7aa413d59206fd511137728df3d9e0fd377429bd/library/std/src/rt.rs:148
   9: 0x55a55759dca5 - main
  10: 0x7f3f93e29d90 - __libc_start_call_main
                at ./csu/../sysdeps/nptl/libc_start_call_main.h:58
  11: 0x7f3f93e29e40 - __libc_start_main_impl
                at ./csu/../csu/libc-start.c:392
  12: 0x55a5573bff25 - _start
  13:        0x0 - <unresolved>'''

With the latest main commit 3edfdc9 it works as expected, presumably because the Manifest::parse method now errors instead of panics and RustdocFromProjectRoot::new ignores any Cargo.toml files with parsing errors.

(But cargo semver-checks check-release --workspace throws an "Error: package bootloader-boot-config not found in /home/philipp/repos/bootloader/common/config" now.)

Expected Behaviour

All other cargo commands work, so I don't understand why the "not all fields of bootloader-boot-config have been present in workspace.package" error occurs. So I would expect that parsing the manifest file succeeds.

Generated System Information

Software version

cargo-semver-checks 0.18.1

Operating system

Linux 6.0.12-76060006-generic

Command-line

/../.cargo/bin/cargo-semver-checks semver-checks --bugreport 

cargo version

> cargo -V 
cargo 1.69.0-nightly (17b3d0de0 2023-02-17)

Compile time information

• Profile: release
• Target triple: x86_64-unknown-linux-gnu
• Family: unix
• OS: linux
• Architecture: x86_64
• Pointer width: 64
• Endian: little
• CPU features: fxsr,sse,sse2
• Host: x86_64-unknown-linux-gnu

Build Configuration

No response

Additional Context

No response

[obi1kenobi]

Hi! Thanks for the detailed bug report, and I'm sorry you ran into this.

I just released v0.18.2 from main with an updated version of the manifest parsing dependency we use, so there's a chance this might be fixed or might behave differently in the new version.

Could I trouble you to try the new version and let me know if it works as expected? If it's still broken, could you post the new error / crash report? It sounds like the issue (if it still exists) is with the manifest-parsing code in our upstream dependency, and if the issue is still present then I'll coordinate with the maintainers of that crate to try to get it fixed.

[phil-opp]

Thanks a lot! I just tried the new version and it indeed solves our main issue. Checking the two main crates of our workspace now works without problems.

However, there is still an error when trying to check the whole workspace using cargo semver-checks check-release --workspace:

Error: package `bootloader-boot-config` not found in /home/philipp/repos/bootloader

I think it has the same cause as the panic in v0.18.1. There still seems to be a parse error of the bootloader-boot-config manifest, it just doesn't cause a panic anymore as the Manifest::parse no longer panics on parsing errors. Instead, the crate is now ignored in the RustdocFromProjectRoot::new function (because of the parsing error), which leads to the "not found" error.

This seems to be an issue in the cargo_toml crate though and not directly related to this crate.

[obi1kenobi]

I'll open an issue in the cargo_toml repo for this. Thanks again!

[phil-opp]

Perfect, thanks!

[obi1kenobi]

The underlying cause is a combination of an unusual repo organization causing a cargo_toml heuristic to misbehave:

• the parent directory of the bootloader-boot-config crate has another Cargo.toml;
• cargo_toml thinks that's the workspace manifest because it's in a parent directory;
• but it's actually a separate crate, hence the parse error.

Moving the bootloader-x86_64-common crate one directory down avoids the problem. Here's the diff — not saying you must apply it, but it's plausible that other tools make a similar assumption to cargo_toml and you might end up finding all those assumptions this way :)

diff --git a/Cargo.toml b/Cargo.toml
index c4cc6e7..8010714 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,7 +12,7 @@ edition = "2021"
 [workspace]
 members = [
     "api",
-    "common",
+    "common/x86_64",
     "common/config",
     "uefi",
     "bios/boot_sector",
@@ -37,7 +37,7 @@ repository = "https://github.com/rust-osdev/bootloader"
 
 [workspace.dependencies]
 bootloader_api = { version = "0.11.0", path = "api" }
-bootloader-x86_64-common = { version = "0.11.0", path = "common" }
+bootloader-x86_64-common = { version = "0.11.0", path = "common/x86_64" }
 bootloader-boot-config = { version = "0.11.0", path = "common/config" }
 bootloader-x86_64-bios-common = { version = "0.11.0", path = "bios/common" }
 
diff --git a/common/Cargo.toml b/common/x86_64/Cargo.toml
similarity index 100%
rename from common/Cargo.toml
rename to common/x86_64/Cargo.toml
diff --git a/common/src/entropy.rs b/common/x86_64/src/entropy.rs
similarity index 100%
rename from common/src/entropy.rs
rename to common/x86_64/src/entropy.rs
diff --git a/common/src/framebuffer.rs b/common/x86_64/src/framebuffer.rs
similarity index 100%
rename from common/src/framebuffer.rs
rename to common/x86_64/src/framebuffer.rs
diff --git a/common/src/gdt.rs b/common/x86_64/src/gdt.rs
similarity index 100%
rename from common/src/gdt.rs
rename to common/x86_64/src/gdt.rs
diff --git a/common/src/legacy_memory_region.rs b/common/x86_64/src/legacy_memory_region.rs
similarity index 100%
rename from common/src/legacy_memory_region.rs
rename to common/x86_64/src/legacy_memory_region.rs
diff --git a/common/src/level_4_entries.rs b/common/x86_64/src/level_4_entries.rs
similarity index 100%
rename from common/src/level_4_entries.rs
rename to common/x86_64/src/level_4_entries.rs
diff --git a/common/src/lib.rs b/common/x86_64/src/lib.rs
similarity index 100%
rename from common/src/lib.rs
rename to common/x86_64/src/lib.rs
diff --git a/common/src/load_kernel.rs b/common/x86_64/src/load_kernel.rs
similarity index 100%
rename from common/src/load_kernel.rs
rename to common/x86_64/src/load_kernel.rs
diff --git a/common/src/logger.rs b/common/x86_64/src/logger.rs
similarity index 100%
rename from common/src/logger.rs
rename to common/x86_64/src/logger.rs
diff --git a/common/src/serial.rs b/common/x86_64/src/serial.rs
similarity index 100%
rename from common/src/serial.rs
rename to common/x86_64/src/serial.rs

(I hereby confirm that the above diff should be considered "intentionally submitted for inclusion in the work" by me, and may be licensed and redistributed under the terms of your projects' licenses.)

[obi1kenobi]

There are still a couple of opportunities for cargo-semver-checks to handle this situation more gracefully, if nothing else then at least displaying a more helpful user-facing message. I'll open a few PRs for that. Thanks again for flagging this.

[obi1kenobi]

Opened https://gitlab.com/crates.rs/cargo_toml/-/issues/23 upstream for the underlying issue.

[phil-opp]

Thanks a lot for looking into this in such detail! This is very useful!

[obi1kenobi]

Happy to help! Thanks for using cargo-semver-checks in your project :)

In the next release (hopefully in ~1hr or so), cargo-semver-checks will report all Cargo.toml parsing failures whenever it fails to find the data for a package it expected to exist. This should make similar future issues easier to diagnose, since the error message won't just be the unhelpful package not found text we saw here.

[obi1kenobi]

https://gitlab.com/crates.rs/cargo_toml/-/issues/23 was just closed as fixed, so I'll include the updated version of cargo_toml (when it's released) in our lockfile and this issue should go away for good. Thanks for your patience!

P.S.: I noticed you have quite a few GitHub sponsors, that's awesome! I recently opened my own GitHub sponsors account where people can support my work on cargo-semver-checks, and I'd love any advice you may have about getting more folks and companies to sponsor me 🙏