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

Document msrv toolchain per chip hal #1058

Merged
merged 1 commit into from
Jan 17, 2024
Merged

Conversation

MabezDev
Copy link
Member

@MabezDev MabezDev commented Jan 2, 2024

image

@MabezDev MabezDev added the skip-changelog No changelog modification needed label Jan 2, 2024
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! Was this + intended?

README.md Show resolved Hide resolved
@SergioGasquez SergioGasquez self-requested a review January 2, 2024 18:39
@jessebraham
Copy link
Member

jessebraham commented Jan 2, 2024

Few things:

  1. There doesn't seem to be a version listed for the esp toolchain, are you proposing we only support the latest release?
  2. The MSRV stated for RISC-V does not match what is in the CI workflow or in the MSRV section of the README
  3. The MSRV section in the README probably can be removed; if we duplicate the info it's almost surely going to fall out of sync at some point

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 3, 2024

I agree having a version on the toolchain also for Xtensa would make sense - then everything is in one place

Dropping the chapter on the MSRV is also probably good since it's almost duplicated then - we will just loose the hint on RUSTC_BOOTSTRAP=1 - not sure if we lose much

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!

@MabezDev
Copy link
Member Author

MabezDev commented Jan 3, 2024

I've made the documentation changes, however, I'm not sure what to do about the actual CI checks.

For reference, the reason I'm adding this info is that any esp-hal version <0.14 will stop working after nightly-2023-11-30 and/or 1.76 stable, but 0.14+ (and the main branch) will work on any compiler post nightly-2023-11-30. Therefore I don't think we actually need to change any CI checks, and when 1.76 rolls around we can go back to stable for our RISC-V targets.

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 - we could mention the thing about versions < 14 and version 14+ and their requirements ... but on the other hand we probably should document the current requirement here

If it causes confusion we can still change it

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.

For now I'll just merge this, and will do a full review of things again before releasing. If we decide to make any changes we can then, not the end of the world.

So, LGTM, thanks!

@jessebraham jessebraham merged commit 4ea4a80 into esp-rs:main Jan 17, 2024
17 checks passed
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.

4 participants