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

Compiler: emit .o file to a temporary location and then atomically rename it #5585

Merged
merged 1 commit into from Jan 15, 2018
Merged

Compiler: emit .o file to a temporary location and then atomically rename it #5585

merged 1 commit into from Jan 15, 2018

Conversation

asterite
Copy link
Member

This was suggested by @RX14

Sometimes we get an error on a compilation that happens after we interrupted a previous compilation. It can happen that a .o file was being generated but it was interrupted, leaving a corrupted file that was later (incorrectly) reused.

This PR changes that so that the object file is first generated with a .o.tmp name, and then moved to the final location with File.rename, which should be an atomic operation.

I tried to reproduce the cache problem but couldn't (it's hard). We can try this fix and see if it happens again. Or if someone knows how to reproduce the problem, they might try to see if this PR fixes it for them.

@asterite asterite self-assigned this Jan 14, 2018
@asterite asterite requested a review from RX14 January 14, 2018 13:50
@ghost
Copy link

ghost commented Jan 14, 2018

I ran into an issue where I used entr -cr together with build --release --lto=thin and got errors after some time, maybe that was related

@RX14
Copy link
Contributor

RX14 commented Jan 14, 2018

@monouser7dig very likely.

@ghost
Copy link

ghost commented Jan 14, 2018

So I could probably try that (if brew --HEAD build works at the moment) or maybe this helps you to reproduce. entr was a fantastic mention from your size btw. thanks, very useful

Copy link
Contributor

@luislavena luislavena left a comment

Choose a reason for hiding this comment

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

I did encounter this a few times. Was unable to replicate after a clean cache and a few break attempts around debugging symbols, but the change to bring atomic creation of object files is welcome 👍

@asterite asterite merged commit 80cbe66 into crystal-lang:master Jan 15, 2018
@asterite asterite added this to the Next milestone Jan 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants