Impl custom tools parsing logic + export PATH creation#85
Impl custom tools parsing logic + export PATH creation#85Vollbrecht merged 7 commits intoesp-rs:masterfrom
Conversation
ivmarkov
left a comment
ivmarkov
left a comment
There was a problem hiding this comment.
I think the change is fine. My only bigger comment is w.r.t. the untyped representation of the tools.json file.
src/espidf.rs
Outdated
|
|
||
| tools_file.read_to_string(&mut tools_string)?; | ||
|
|
||
| let parsed_file = serde_json::from_str::<serde_json::Value>(&tools_string)?; |
There was a problem hiding this comment.
Why are you parsing the tools.json file as an untyped map?
I would assume it would still make sense to have an internal struct named ToolsJson or suchlike, which is strongly typed?
There was a problem hiding this comment.
Yeah, i was a bit lazy here. I think there is not to much benefit here for creating a separate strongly typed version over the raw serialized data ( skipping stuff that are not needed), but i can implement that so no problem there.
There was a problem hiding this comment.
Up to you, but it would lower the amounts of unwraps in this code considerably, I think.
If you decide not to, just merge.
There was a problem hiding this comment.
ok i addressed that, shooting with big guns now. 🔫 In the process i found out multiple problems in the json schema used by espressif themself. No unwraps anymore ( at least as far as the schema is concerned).
src/espidf.rs
Outdated
|
|
||
| let version_cmd_args = tool_object["version_cmd"] | ||
| .as_array() | ||
| .unwrap() |
There was a problem hiding this comment.
These unwraps here and below are precisely because the tool_object is untyped.
src/espidf.rs
Outdated
| "linux" => match arch { | ||
| "x86_64" => Some("linux-amd64"), | ||
| // TODO add and test arm variants | ||
| _ => None, |
There was a problem hiding this comment.
Might want to at least add support for RPI?
There was a problem hiding this comment.
arm is kinda a wormhole on linux ;D for rpi 3 & 4 & 5 i think we need different targets and it depends on what OS are flashed onto the pi. E.g a stock "pi os" or some debian version. I think they are all "linux" on the os, but the second part is more unclear. Do you know working matching patterns for the "arch" variants for the different thumb and aarach variants on pi3 -pi5. IIrc they are all different.
There was a problem hiding this comment.
added linux arm variants the way i think its correct
The custom tools parsing logic introduced in esp-rs#85 does not process the overrides in tools.json. This omission breaks compilation on macOS and likely several other platforms. This commit adds override parsing logic to properly handle overrides from tools.json Signed-off-by: Tom Stokes <tomstokes@radixengineering.com>
The custom tools parsing logic introduced in esp-rs#85 has incorrect ARCH values. The list of possible ARCH values can be found in the Rust documentation: https://doc.rust-lang.org/std/env/consts/constant.ARCH.html Unfortunately, the Rust standard library doesn't distinguish between armel and armhf for 32-bit ARM, so this defaults to armel for 32-bit ARM. The previous code would not properly match any ARM platforms (armv7 and armv8 are not possible ARCH values) and was missing 32-bit Linux. Signed-off-by: Tom Stokes <tomstokes@radixengineering.com>
The custom tools parsing logic introduced in esp-rs#85 has incorrect ARCH values. The list of possible ARCH values can be found in the Rust documentation: https://doc.rust-lang.org/std/env/consts/constant.ARCH.html Unfortunately, the Rust standard library doesn't distinguish between armel and armhf for 32-bit ARM, so this defaults to armel for 32-bit ARM. The previous code would not properly match any ARM platforms (armv7 and armv8 are not possible ARCH values) and was missing 32-bit Linux. Signed-off-by: Tom Stokes <tomstokes@radixengineering.com>
* Process overrides in custom tools logic The custom tools parsing logic introduced in #85 does not process the overrides in tools.json. This omission breaks compilation on macOS and likely several other platforms. This commit adds override parsing logic to properly handle overrides from tools.json Signed-off-by: Tom Stokes <tomstokes@radixengineering.com> * Fix platform matching in tools parsing The custom tools parsing logic introduced in #85 has incorrect ARCH values. The list of possible ARCH values can be found in the Rust documentation: https://doc.rust-lang.org/std/env/consts/constant.ARCH.html Unfortunately, the Rust standard library doesn't distinguish between armel and armhf for 32-bit ARM, so this defaults to armel for 32-bit ARM. The previous code would not properly match any ARM platforms (armv7 and armv8 are not possible ARCH values) and was missing 32-bit Linux. Signed-off-by: Tom Stokes <tomstokes@radixengineering.com> * Fix clippy warning with 1.79.0 toolchain The newest version of clippy catches a clippy::manual-unwrap-or-default issue, causing CI builds to fail. This implements the recommended fix and therefore should fix the CI builds. Signed-off-by: Tom Stokes <tomstokes@radixengineering.com> --------- Signed-off-by: Tom Stokes <tomstokes@radixengineering.com>
2 participants
This fixes the installation of esp-idf tools in esp-idf version >= 5.3. This is accomplished by stop relighting on the tools.py "export" command and instead parse and evaluate the tools.json file found in esp-idf ourselfs. Previously embuild relight on broken output of the "export" command to get all necessary information. This was fixed upstream and we cant use it the way we did previously.
The tools.json file is the source of truth for the tools.py installation script and is used in this tool now to get the equivalent output needed. E.g providing the python virt_env and the path to all installed tools ( compiler, ninja, cmake etc)