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

Fix race condition in clang_macro_fallback #3111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HKalbasi
Copy link
Member

@HKalbasi HKalbasi commented Feb 2, 2025

Previously, bindgen by default used the same directory of the headers as the directory for temporary files of the clang_macro_fallback feature. Aside from its other problems like making temporary noises in git or failing to remove these files on failing or aborted builds, this caused a race condition in build of retina that I spend two DAYS to figure it out.

The problem is, if you run bindgen on the same set of files multiple times simultaneously (which happened in retina since a crate was included two times in the crate graph due different features, and it ran bindgen in its build script) one of them can remove clang_macro_fallback files of the other one and break it.

I fixed it by moving the files generated by clang_macro_fallback in a new temp directory each time bindgen is invoked.

@HKalbasi HKalbasi force-pushed the fix-race-condition branch 2 times, most recently from fabe930 to b002407 Compare February 2, 2025 22:30
@ojeda
Copy link
Contributor

ojeda commented Feb 4, 2025

Cc @jbaublitz since he is probably interested.

@jbaublitz
Copy link
Contributor

@HKalbasi You may want to base this PR on #3072 as that is the PR I put up to address some problems that @ojeda bumped into while using the clang fallback code.

@HKalbasi
Copy link
Member Author

HKalbasi commented Feb 7, 2025

I will do the rebase after your PR is merged so I don't have to rebase it multiple times in case your PR needs further changes, and to keep the diff clean if you or anyone else want to review it in the meantime.

@jbaublitz
Copy link
Contributor

@HKalbasi Okay, it looks like the PR got merged.

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.

3 participants