Skip to content

Update C2Rust to 0.16#195

Merged
bors[bot] merged 4 commits intoRIOT-OS:masterfrom
chrysn-pull-requests:new-c2rust-on-focal
May 13, 2022
Merged

Update C2Rust to 0.16#195
bors[bot] merged 4 commits intoRIOT-OS:masterfrom
chrysn-pull-requests:new-c2rust-on-focal

Conversation

@chrysn
Copy link
Member

@chrysn chrysn commented May 13, 2022

Things got a lot easier since upstream did awesome work leading up to immunant/c2rust#365.

This release allows riot-sys to do away with a quite a few workarounds, and resolves a blocker for RIOT-OS/RIOT#18056 in passing.

This is a prerequisite for #189 (in the course of which C2Rust can then be simplified further, as we can just use Rust from Ubuntu to build it, or even simply let this become a Debian package over time).

chrysn added 3 commits May 13, 2022 10:14
Installation via rustup is left in place to make backporting to focal
easy. (MSRV is 1.58 which is present in Ubuntu LTS starting with Jammy).
@chrysn
Copy link
Member Author

chrysn commented May 13, 2022

bors try

(I'd like to see whether this builds fine with the currently pushed preliminary c2rust image; once this is approved but before it is m3rged, I'd re-tag that and remove the top commit that says "DO NOT M3RGE")

PS. I'm just spelling weirdly to not give b0rs any weird ideas on which keywords to match.

bors bot added a commit that referenced this pull request May 13, 2022
@chrysn chrysn changed the title Update C2Rust Update C2Rust to 0.16 May 13, 2022
@bors
Copy link
Contributor

bors bot commented May 13, 2022

try

Build succeeded:

@chrysn
Copy link
Member Author

chrysn commented May 13, 2022

This is still running into trouble on RISC-V (which I could previously not test because its C side requires building in docker on my system), investigating... (Plus: Why didn't this show here during the "Rust build test" phase?)

@chrysn
Copy link
Member Author

chrysn commented May 13, 2022

On a side note, now that things work without an extra Rust toolchain, we could simplify things by just doing the c2rust build inside riotbuild. (This was originally impossible because we needed an odd nightly). There is still an upside to the current approach, that is, that the Debian package around it makes it easy to remove build dependencies later while keeping runtime dependencies.

If we dropped the current mechanism, we'd wind up with libclang-dev, libssl-dev and llvm-dev installed in the containers. According to apt, that comes at a cost of 56MB (downloads, that's probably also the compressed delta) / 424MB (uncompressed). They could be uninstalled again after building c2rust, but that'd need careful consideration of which runtime components need to be retained (which is what the Debian package does for us).

The PR works as it is now (also for RISC-V at the cost of using a git version rather than the released one again, but now that's easier); if the reviewers want to get rid of the complexity of the extra docker image, I can shift over to doing it in riotdocker right away.

@kaspar030
Copy link
Contributor

if the reviewers want to get rid of the complexity of the extra docker image, I can shift over to doing it in riotdocker right away.

the reviewers might like that c2rust doesn't get rebuilt so much. :)

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030
Copy link
Contributor

bors merge

bors bot added a commit that referenced this pull request May 13, 2022
195: Update C2Rust to 0.16 r=kaspar030 a=chrysn

Things got a lot easier since upstream did awesome work leading up to immunant/c2rust#365.

This release allows riot-sys to do away with a quite a few workarounds, and resolves a blocker for RIOT-OS/RIOT#18056 in passing.

This is a prerequisite for #189 (in the course of which C2Rust can then be simplified further, as we can just use Rust from Ubuntu to build it, or even simply let this become a Debian package over time).

Co-authored-by: chrysn <chrysn@fsfe.org>
@kaspar030
Copy link
Contributor

did you consider PRing the debian package generation upstream?

@chrysn chrysn force-pushed the new-c2rust-on-focal branch from 938c043 to 1287480 Compare May 13, 2022 13:51
@bors
Copy link
Contributor

bors bot commented May 13, 2022

Canceled.

@kaspar030
Copy link
Contributor

ok :)

bors merge

@chrysn
Copy link
Member Author

chrysn commented May 13, 2022

I had to force push once to get rid of the "DO NOT MERGE" commit that had previously ensured that the preview c2rust image was pushed; now that I pushed to the right tag (after approval of the PR) things can build as was before. Therefore,

bors merge

again.

the reviewers might like that c2rust doesn't get rebuilt so much. :)

Trust me, so would I. That now it is running on stable removes a lot of volatility. Can't promise that this is the last for some time, due to the RISCV bug I missed in 0.16, but I do hope it's getting better.

If we had better sharing of built code between examples, we might even get rid of it as a binary completely (and have Cargo build it as build-dependency locally on demand).

did you consider PRing the debian package generation upstream?

Yes -- but in the end the place where that should go is Debian directly.

@bors
Copy link
Contributor

bors bot commented May 13, 2022

Already running a review

@bors
Copy link
Contributor

bors bot commented May 13, 2022

Build succeeded:

@bors bors bot merged commit 8ee87ad into RIOT-OS:master May 13, 2022
@chrysn chrysn deleted the new-c2rust-on-focal branch May 13, 2022 14:29
@chrysn chrysn mentioned this pull request May 13, 2022
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.

2 participants