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

Updates for Xtensa enabled 1.83 compiler #2615

Merged
merged 14 commits into from
Dec 17, 2024
Merged

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Nov 26, 2024

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Please provide a clear and concise description of your changes, including the motivation behind these changes. The context is crucial for the reviewers.

Testing

Describe how you tested your changes.

@MabezDev MabezDev added the skip-changelog No changelog modification needed label Nov 26, 2024
@MabezDev MabezDev changed the title Test Xtensa enabled 1.83 compiler Updates for Xtensa enabled 1.83 compiler Nov 28, 2024
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@@ -772,7 +772,7 @@ fn lint_package(path: &Path, args: &[&str], fix: bool) -> Result<()> {
// build in release to reuse example artifacts
let cargo_args = builder.arg("--release");
let cargo_args = if fix {
cargo_args.arg("--fix").arg("--lib")
cargo_args.arg("--fix").arg("--lib").arg("--allow-dirty")
Copy link
Member

Choose a reason for hiding this comment

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

Thank you, keep forgetting to add this 😅

@jessebraham
Copy link
Member

Any updates here?

@MabezDev
Copy link
Member Author

MabezDev commented Dec 6, 2024

Nothing yet, the LLVM issue is being looked into still. Our last resort is still available, which is to bump the MSRV.

@MabezDev
Copy link
Member Author

MabezDev commented Dec 9, 2024

Issue is fixed 🎉 , needs to make its way to github and I'll start the release builds

@MabezDev MabezDev force-pushed the compiler-test branch 2 times, most recently from 8bf4031 to 41a877b Compare December 9, 2024 11:14
@MabezDev
Copy link
Member Author

Well, it's fixed... but we're still in a situation where MSRV is failing because the bug that I found whilst trying to avoid bumping MSRV also affects the MSRV toolchain 🙃.

I think we have a few options here:

  • Bump MSRV + keep these changes (no more naked fns)
  • Bump MSRV + revert changes to xtensa lx (back to naked fns)
  • Backport the patch to $MSRV toolchain and cut a release from there

Thoughts?

@bjoernQ
Copy link
Contributor

bjoernQ commented Dec 10, 2024

Getting rid of naked functions sounds best to me.

Not ideal to bump MSRV to 1.83 however.

Not sure how much effort option 3 would be

@MabezDev MabezDev marked this pull request as ready for review December 17, 2024 11:24
@MabezDev
Copy link
Member Author

I went for the first option. When CI is green, I'll promote the compiler release to stable and remove the hardcoded versions from the workflows here.

.github/workflows/hil.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@bjoernQ bjoernQ left a comment

Choose a reason for hiding this comment

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

LGTM - I also briefly tested with some esp-wifi examples and they still work fine

Copy link
Member

@jessebraham jessebraham 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!

@jessebraham
Copy link
Member

(@bugadani will review this too, please don't merge yet)

Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

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

ASM changes look OK.

Crate MSRVs are still set to 1.79, is that intentional? MSRV bump is also missing from the changelogs - although the label is set, we used to call out these changes.

e.g. in esp-hal:

rust-version = "1.79.0"

@MabezDev
Copy link
Member Author

Thanks, I always forget about the rust-version field. Should be fixed now, along with some changelog entries.

@jessebraham jessebraham added this pull request to the merge queue Dec 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2024
@bugadani bugadani added this pull request to the merge queue Dec 17, 2024
Merged via the queue into esp-rs:main with commit 85d30e9 Dec 17, 2024
28 checks passed
@tommasoclini
Copy link
Contributor

All of my xtensa projects are failing to build, do I have to use the github repo as the source for the esp-hal package? For how long?

@bugadani
Copy link
Contributor

Please kindly downgrade your compiler with espup install --toolchain 1.82.0.1 until we resolve this incompatibility.

@tommasoclini
Copy link
Contributor

Please kindly downgrade your compiler with espup install --toolchain 1.82.0.1 until we resolve this incompatibility.

Thx, I didn't know I could install different versions of tool chains with espup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants