Skip to content

Simplify c2rust#196

Closed
chrysn wants to merge 5 commits intoRIOT-OS:masterfrom
chrysn-pull-requests:simplify-c2rust
Closed

Simplify c2rust#196
chrysn wants to merge 5 commits intoRIOT-OS:masterfrom
chrysn-pull-requests:simplify-c2rust

Conversation

@chrysn
Copy link
Member

@chrysn chrysn commented May 13, 2022

This is the change hinted at in #195 (comment) -- it does away with all the troublesome complexity (different images whose dependencies are intransparent to the CI builds).

On the downside, it increases the compressed image size by 56MB (estimated). The reduction in complexity should be well worth it. Compared to before (when a C2Rust update was an exercise of confirming a PR without merging it, then updating a docker tag manually, then letting the actual build through in a way that's not trivial to roll back), the whole thing is now just a block of dependencies and a single line of installation.

Also, this is sneaking in a fixup in our branch of C2Rust that allows unblocking RIOT-OS/RIOT#18056 for good now.

chrysn added 2 commits May 13, 2022 20:21
This has become possible now that it doesn't need fancy build
dependencies any more.
This unblocks a stage of [#18056].

[#18056]: RIOT-OS/RIOT#18056
@chrysn chrysn force-pushed the simplify-c2rust branch from 5e88487 to 792353c Compare May 13, 2022 18:54
chrysn added 3 commits May 13, 2022 21:29
This is required to make any `cargo install` work given the locations.
@chrysn
Copy link
Member Author

chrysn commented Jun 1, 2022

Unless urgency comes to the blocked issue, I'm not pushing this actively until immunant push their next C2Rust release, as that'd allow further simplification. (The C2Rust version referenced in this PR is still a branch of mine; they already merged that, but I can't easily reference "the current master they have", and when they release it'll be shorter again).

Once that's through I should also update installation docs (they still refer to an older C2Rust version, which should still work though, as the workarounds are not removed yet).

[edit: I could have, see below]

@chrysn
Copy link
Member Author

chrysn commented Jun 14, 2022

I'd prefer to have #199 merged instead; this is only left open in case something unexpected pops up there. (I'm running a recent C2Rust version locally but not the exact one I'm pinning there, but then again the changes appear to be trivial)

@chrysn
Copy link
Member Author

chrysn commented Jun 14, 2022

Also, I have reason to believe that if this PR were started again, it'd fail with similar symptoms as #199; abandoning this in full.

@chrysn chrysn closed this Jun 14, 2022
bors bot added a commit that referenced this pull request Jun 14, 2022
199: Simplify c2rust (with latest C2Rust version) r=kaspar030 a=chrysn

This is equivalent to #196 (also fixing RIOT-OS/RIOT#18056) except that it is

* rebased already, and
* uses the latest upstream version after upstream merged all my relevant pull requests (they're still a bit slow on doing a Cargo release, but installing from a rev in their git history is actually easy).

(Opened as a separate PR because if this fails for some reason, it's easy to fall back to #196).

Co-authored-by: chrysn <chrysn@fsfe.org>
@chrysn chrysn deleted the simplify-c2rust branch June 14, 2022 21:00
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.

1 participant