Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always resolve package_id from metadata when finding bootloader and partition table #632

Merged

Conversation

kyrias
Copy link
Contributor

@kyrias kyrias commented Apr 29, 2024

The package ID format was officially documented as an opaque identifier and the only thing we were allowed to do with them is look them up in the cargo metadata. Rust 1.77 switched the package ID field to use the publicly documented "Package ID Specification" format instead, which means that the old logic no longer works.

@kyrias kyrias force-pushed the fix-bl-and-pt-finding-since-rust-1.77 branch from 8ee9bc3 to 4cc5bdb Compare April 29, 2024 11:48
@kyrias
Copy link
Contributor Author

kyrias commented Apr 29, 2024

Turns out that it's only things built by -Zbuild-std which end up not existing in the metadata, so I'm going to rework this to always look it up there instead.

@kyrias kyrias force-pushed the fix-bl-and-pt-finding-since-rust-1.77 branch from 4cc5bdb to 4ccd858 Compare April 29, 2024 15:08
@kyrias kyrias changed the title Make bootloader and partition table finding work on Rust >=1.77 and esp-idf-sys >=0.34.0 Always resolve package_id from metadata when finding bootloader and partition table Apr 29, 2024
Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Changes LGTM! Just left one nitpick suggestion (sorry). I was able to build both a std and a no_std with your changes. Mind elaborating which further testing you did?

CHANGELOG.md Outdated Show resolved Hide resolved
…artition table

The package ID format was officially documented as an opaque identifier
and the only thing we were allowed to do with them is look them up in
the cargo metadata.  Rust 1.77 switched the package ID field to use the
publicly documented "Package ID Specification" format instead, which
means that the old logic no longer works.

Signed-off-by: Johannes Löthberg <johannes.loethberg@elokon.com>
@kyrias kyrias force-pushed the fix-bl-and-pt-finding-since-rust-1.77 branch from 4ccd858 to 612fdab Compare May 6, 2024 09:20
@kyrias
Copy link
Contributor Author

kyrias commented May 6, 2024

I was able to build both a std and a no_std with your changes. Mind elaborating which further testing you did?

Compile-testing isn't enough because they build fine, but they silently fail to find the bootloader and partition table and silently fall back to the ones built into the binary. To test it properly the simplest way is to flash it and look that it's using the correct partition table when it boots. The bootloader is a bit trickier, though if one was found the other would've been found as well, but to explicitly test it the simplest way would be to add an extra log line to the bootloader and see that it's printed, I guess.

Sadly there's no way to see from the espflash side whether it used the built-in ones instead. It would actually be nice if it was possible to configure it to never use the built-in ones and instead return an error if it can't find a custom one, because if you have a custom partition table it would definitely be an error condition to suddenly build without one.

@SergioGasquez
Copy link
Member

Sadly there's no way to see from the espflash side whether it used the built-in ones instead.

IIRC, if you enable debug logs RUST_LOG=debug espflash ..., you should be able to see which bootloader/partition table are being used.

@kyrias
Copy link
Contributor Author

kyrias commented May 6, 2024

Sadly there's no way to see from the espflash side whether it used the built-in ones instead.

IIRC, if you enable debug logs RUST_LOG=debug espflash ..., you should be able to see which bootloader/partition table are being used.

Not with cargo-espflash at least. It just logs the command line arguments and the config, but we don't configure espflash directly, we use the ESP_IDF_GLOB_XXX functionality to copy the partition table into the esp-idf build directory.

Copy link
Member

@SergioGasquez SergioGasquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks

@SergioGasquez SergioGasquez merged commit 3497701 into esp-rs:main May 14, 2024
28 checks passed
@kyrias kyrias deleted the fix-bl-and-pt-finding-since-rust-1.77 branch May 14, 2024 23:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants