Improve app-descriptor detection #975
Conversation
|
Why the "for unknown reasons" part? https://doc.rust-lang.org/cargo/reference/profiles.html#strip says:
|
ah yeah ... ok makes a lot of sense - I assumed (without checking) it strips just debuginfo |
bjoernQ
force-pushed
the
improve-app-desc-check
branch
from
0aa9a7a to
7414168
Compare
| } | ||
|
|
||
| /// Check if the provided ELF contains the app descriptor required by [the IDF bootloader](https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-guides/bootloader.html). | ||
| pub fn check_idf_bootloader(elf_data: &Vec<u8>) -> Result<()> { |
There was a problem hiding this comment.
I've just tried using this function in probe-rs to fix probe-rs/probe-rs#3564 and come on why are we returning a miette diagnostic from this...?
There was a problem hiding this comment.
we should change that! maybe there are more places like this?
There was a problem hiding this comment.
but ... probably for 5.x since this is already published public API :(
There was a problem hiding this comment.
OK I've managed to work around it by this line - turning the diagnostic back into a string, and wrapping it into the espflash::Error::AppDescriptorNotPresent error, which it was supposed to be in the first place...
There was a problem hiding this comment.
We either shouldn't use miette in the library, or embrace it and don't expose espflash's error type, but this hybrid mess is just sad.
but ... probably for 5.x since this is already published public API :(
We can extract the implementation into another function that returns the Result<(), Error> type, and just call that().into_diagnostic() in the current function.
There was a problem hiding this comment.
We either shouldn't use miette in the library, or embrace it and don't expose espflash's error type, but this hybrid mess is just sad.
+1 ... we probably shouldn't embrace it but scrape it from the API surface
We can extract the implementation into another function that returns the Result<(), Error> type, and just call that().into_diagnostic() in the current function.
sure - and deprecate the current function for now
Might be worth a separate issue ... pretty sure comments on a merged PR will be lost in an instant
A Rust project with
strip = truewill discard the symbol we are looking for but the section is kept.See #927 (comment)
This should fix that situation