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

Add support for using custom cargo metadata when in a workspace #300

Merged
merged 2 commits into from
Dec 1, 2022

Conversation

jessebraham
Copy link
Member

Alright, let's try this again 😁

I took the advice of @liebman and used cargo in place of the cargo_toml package, and this has resulted in a much more robust and flexible solution. Not entirely happy with the load_metadata function still, but it appears to work at least.

I did actually test this on hardware this time around, and it seems to be working as intended. @liebman if you could test this out one more time that would be appreciated!

Also cleaned up the errors module, bumped a couple dependencies, and some other little bits.

(Hopefully) closes #291

@liebman
Copy link

liebman commented Nov 16, 2022

Using a small test project I tested running at the projects root and in the binary crates directory.

I found that this needs to use a newer cargo than 0.63 or it does not have support for workspace inheritance and you get this

Error: cargo_espflash::invalid_workspace

  × The current workspace is invalid, and could not be loaded
  help: Ensure that a valid Cargo.toml file is in the executing directory

If I update it to 0.66 (latest) it works well. With one minor issue.

  • Running from root works well (with .cargo/config.tom at workspace root level)
  • Running from the binary crate fails if there is no .cargo/config.toml in the current directory as it can't find the target (it does not look in the workspace root directory)

Basically there currently has to be a .cargo/config.toml in the current directory to specify a target. Ideally it would look first in the package that will be flashed (--package option if running from root) and if one is not found there then the workspace root.

I see you have a comment about having to use an older version - would not that only apply if these were going to be run on an esp32 target?

@jessebraham
Copy link
Member Author

jessebraham commented Nov 30, 2022

Sorry for the delays, I needed a little break from this project 😅

Thank you for providing the example project. I have made a number of additional changes and I am able to flash your project both from the workspace root and from within the candle package, so I think we're in good shape! @liebman if you wouldn't mind just giving this one more test to verify it's working on your end that would be appreciated!

EDIT: Oops, cargo is unhappy about our MSRV. Will fix.

@liebman
Copy link

liebman commented Nov 30, 2022

This works!

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, and seems to be working fine! Thanks for taking care of this!

@jessebraham jessebraham merged commit 0dfeff0 into esp-rs:main Dec 1, 2022
@jessebraham jessebraham deleted the fixes/workspace branch December 1, 2022 16:03
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.

can't specify partition table in a workspace
3 participants