Skip to content

Use standard wording for the MSRV note #436

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

Merged
merged 2 commits into from
Apr 20, 2020
Merged

Conversation

teskje
Copy link
Contributor

@teskje teskje commented Apr 18, 2020

Part of wg/#445.

svd2rust already has its MSRV documented and tested, but @adamgreig suggested updating it to use the standard wording. Since this is a special case in that the MSRV applies to the generated code, not svd2rust itself, the standard wording doesn't fully apply, so I changed it in a (hopefully) sensible way.

An open question is: Should we also document the MSRV of svd2rust itself? Looking at how the CI works, currently it's implicitly guaranteed to be the same version als the MSRV for the generated code, so we could just extend the note in the README to also include svd2rust itself.

@teskje teskje requested a review from a team as a code owner April 18, 2020 15:41
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ryankurte (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools labels Apr 18, 2020
Copy link

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

bors r+

bors bot added a commit that referenced this pull request Apr 19, 2020
436: Use standard wording for the MSRV note r=ryankurte a=ra-kete

Part of [wg/#445](rust-embedded/wg#445).

svd2rust already has its MSRV documented and tested, but @adamgreig suggested updating it to use the standard wording. Since this is a special case in that the MSRV applies to the generated code, not svd2rust itself, the standard wording doesn't fully apply, so I changed it in a (hopefully) sensible way.

An open question is: Should we also document the MSRV of svd2rust itself? Looking at how the CI works, currently it's implicitly guaranteed to be the same version als the MSRV for the generated code, so we could just extend the note in the README to also include svd2rust itself.

Co-authored-by: Jan Teske <[email protected]>
@bors
Copy link
Contributor

bors bot commented Apr 19, 2020

Build failed:

@teskje
Copy link
Contributor Author

teskje commented Apr 19, 2020

The built failed because the i686-apple-darwin Target is no longer fully supported: https://blog.rust-lang.org/2020/01/03/reducing-support-for-32-bit-apple-targets.html

I suggest we simply remove it from the CI, unless there is a reason svd2rust still needs to support it?

@therealprof
Copy link
Contributor

@ra-kete Yeah, let's remove it. That target is pretty silly anyway: pretty much only the very first MacBooks had only 32bit Intel MCUs, everything else had 64Bit MCUs from the start and 64Bit support was there since Lion (2011). That target was useful for legacy code bases which I highly doubt even exist since Rust 1.0 was released 4 years after Apple started actually requiring 64bit capable CPUs...

@therealprof
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 20, 2020

Build succeeded:

@bors bors bot merged commit a0618f9 into rust-embedded:master Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants