Skip to content

Allow running multiple transpiles in parallel #1241

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 1 commit into from
May 10, 2025

Conversation

jameysharp
Copy link
Contributor

If c2rust-transpile needs to generate a temporary compile_commands.json, previously it used a fixed path for it across all invocations. Instead, create a uniquely-named temporary directory for it, so that parallel transpiles don't clobber each other.

I've used a two-year-old release of tempfile to avoid adding a bunch of duplicate versions of other crates to Cargo.lock, but presumably some upgrades ought to happen sooner or later.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this! Some small changes preferred, but the tempdir part looks good.

@kkysen
Copy link
Contributor

kkysen commented May 9, 2025

I've used a two-year-old release of tempfile to avoid adding a bunch of duplicate versions of other crates to Cargo.lock, but presumably some upgrades ought to happen sooner or later.

This is fine. We're (@joshtriplett is) in the process of splitting out the unstable crates (#1227, #1220) so that we can freely update our dependencies for the stable crates like c2rust-transpile, among other benefits, so we should be able to upgrade dependencies like tempfile soon enough.

If c2rust-transpile needs to generate a temporary compile_commands.json,
previously it used a fixed path for it across all invocations. Instead,
create a uniquely-named temporary directory for it, so that parallel
transpiles don't clobber each other.

I've used a two-year-old release of tempfile to avoid adding a bunch of
duplicate versions of other crates to Cargo.lock, but presumably some
upgrades ought to happen sooner or later.
@jameysharp
Copy link
Contributor Author

Thanks for reviewing! I've made the changes you requested. I don't understand why you prefer buffering the entire temporary compile_commands.json in memory instead of using a fixed-size buffer, but that's okay, I've reverted that change as you asked. How does this look to you now?

I tried to understand why the c2rust-testsuite CI task failed last time but it didn't look like it had anything to do with my changes, I think. Is that an existing issue?

By the way, I was looking at this while trying to figure out how to introduce snapshot testing for c2rust-transpile, because I found that approach super helpful when working on Wasmtime. Now I see you've been discussing that recently in #1218 (comment) so I'm glad to see that might be welcome.

@kkysen
Copy link
Contributor

kkysen commented May 10, 2025

Thanks for reviewing! I've made the changes you requested. I don't understand why you prefer buffering the entire temporary compile_commands.json in memory instead of using a fixed-size buffer, but that's okay, I've reverted that change as you asked. How does this look to you now?

Thanks! Looks all good. Buffering it in memory and writing in one go is a bit simpler and since the compile commands will be very small, it's not really gonna make a difference. It'll probably be marginally faster, but that's not really very important here; it's moreso that it's just a bit simpler.

I tried to understand why the c2rust-testsuite CI task failed last time but it didn't look like it had anything to do with my changes, I think. Is that an existing issue?

Yeah, that's an existing issue that's been failing on curl for a little while. We're still trying to figure that one out, but I haven't had too much time to look into it more lately.

By the way, I was looking at this while trying to figure out how to introduce snapshot testing for c2rust-transpile, because I found that approach super helpful when working on Wasmtime. Now I see you've been discussing that recently in #1218 (comment) so I'm glad to see that might be welcome.

That'd definitely be welcome! We have a few insta snapshot tests for some newer tests, but if you'd like to help migrate the older transpiler tests, that'd be super helpful.

@kkysen kkysen merged commit 0757fe4 into immunant:master May 10, 2025
8 of 9 checks passed
@jameysharp jameysharp deleted the unique-temp-path branch May 10, 2025 08:36
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