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

local ports: use cache lock (#9342) #9391

Merged
merged 2 commits into from Sep 9, 2019

Conversation

Beuc
Copy link
Contributor

@Beuc Beuc commented Sep 5, 2019

Cf. #9342

shared.try_delete(fullname)
shutil.copytree(path, os.path.join(fullname, subdir))
Ports.clear_project_build(name)
return
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you leave the return statement outside (after) the try/finally.. it makes the early return pattern more clear.

Also, the the first two lines of the try block and stay outside too I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

logger.warning('grabbing local port: ' + name + ' from ' + path + ' to ' + fullname + ' (subdir: ' + subdir + ')')
shared.try_delete(fullname)
shutil.copytree(path, os.path.join(fullname, subdir))
Ports.clear_project_build(name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Presumably its these three lines that require the lock?

Copy link
Contributor Author

@Beuc Beuc Sep 8, 2019

Choose a reason for hiding this comment

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

The SDL2 port deletes depending ports prior (re)building, so I'm afraid that locking only those lines will lead to build failures like SDL2 (process1) > SDL2_image (p1) > SDL2 (process2) > missing SDL2_image.
That's basically why I didn't try to be specific about the lock.
I cannot confirm right now due to #9402.
EDIT: clarified my failure scenario, needs some testing

Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable to me. Basically, any local-ports work should be locked.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm % comments

@kripken
Copy link
Member

kripken commented Sep 9, 2019

Something weird happened with the tests here, but this should be safe to merge...

@kripken kripken merged commit 4492aa5 into emscripten-core:incoming Sep 9, 2019
@Beuc Beuc deleted the patch-9342 branch September 11, 2019 22:08
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
)

Locking around local ports operations avoids possible race conditions.
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.

None yet

3 participants