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

[RSDK-8992] Upgrade to rust 1.82 #354

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

acmorrow
Copy link
Member

Rust 1.82 is the latest that esp-rs offers.

@acmorrow acmorrow requested a review from a team as a code owner November 26, 2024 17:24
Copy link
Member Author

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

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

Some notes. Also, need to think about how to get this green in CI because right now it fails in anything using the docker image since they haven't been updated to 1.82, but we would need to merge this in order to build new docker images that had 1.82, so it is a bit of a chicken/egg thing.

@@ -1,23 +1,23 @@
[target.xtensa-esp32-espidf]
linker = "ldproxy"
runner = "espflash --monitor"
rustflags = ["-C", "default-linker-libraries", "-Clink-args=-Wl,-Map=./esp-build.map"]
rustflags = ["-C", "default-linker-libraries", "-Clink-args=-Wl,-Map=./esp-build.map", "--cfg", "espidf_time32"]
Copy link
Member Author

Choose a reason for hiding this comment

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

Required due to libc crate upgrade and rust-lang/libc#3993.

@@ -39,7 +39,7 @@ debug = true # Symbols are nice and they don't increase the size on Flash
opt-level = "s"

[workspace.dependencies]
anyhow = "1.0.89"
anyhow = "1.0.93"
Copy link
Member Author

Choose a reason for hiding this comment

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

Updates to this section per cargo upgrade with rust 1.82 enabled.

cbindgen = "0.27.0"
chrono = "0.4.38"
chrono-tz = "0.10.0"
clap = "4.5.20"
clap = "4.5.21"
Copy link
Member Author

Choose a reason for hiding this comment

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

⬇️ - I had hoped to upgrade embedded-svc and esp-idf-svc, but did not succeed due to a puzzling error. I'll take another go at it, since I believe all that was blocking it was the rust version.

{
println!("cargo:rustc-check-cfg=cfg(has_robot_config)");
Copy link
Member Author

Choose a reason for hiding this comment

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

Without this, newer clippy yelled at me about cfg(has_robot_config) as an unknown config check.

@@ -18,8 +18,10 @@ pub struct Cloud {
}

fn main() {
#[cfg(not(feature = "test"))]
#[cfg(not(test))]
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was an error before? I'm not sure.

@@ -1,6 +1,7 @@
use regex::Regex;

fn main() {
println!("cargo::rustc-check-cfg=cfg(esp32)");
Copy link
Member Author

Choose a reason for hiding this comment

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

Like above, clippy would complain that this was an unknown config in esp32/board.rs.

} else {
vec![]
};
let i2c_confs = cfg
Copy link
Member Author

Choose a reason for hiding this comment

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

Newer clippy suggestion

@@ -115,7 +115,7 @@ impl DoCommand for Coredump {
command_struct: Option<protobuf::Struct>,
) -> Result<Option<protobuf::Struct>, crate::common::generic::GenericError> {
if let Some(cmd) = command_struct {
if cmd.fields.get("sizes").is_some() {
if cmd.fields.contains_key("sizes") {
Copy link
Member Author

Choose a reason for hiding this comment

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

This and ⬇️ are from newer clippy suggestions

Copy link
Member

@mattjperez mattjperez left a comment

Choose a reason for hiding this comment

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

LGTM

@acmorrow
Copy link
Member Author

I built the docker containers and validated a build within it, so I think that aspect is good to go.

@acmorrow
Copy link
Member Author

FYI I'm not planning to merge this until next week

Copy link
Member

@gvaradarajan gvaradarajan left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants