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

Use an advisory lock to co-ordinate access to git checkout repos #3592

Merged
merged 3 commits into from
Dec 15, 2022

Conversation

mitchmindtree
Copy link
Contributor

@mitchmindtree mitchmindtree commented Dec 14, 2022

This introduces an advisory lock around the re-usable git checkout repo directories.

Closes #2603, #3484.

Unfortunately, it's a little tricky to reliably test this from this project repository as doing so requires creating a test project with one or more git dependencies. We can't reliably add git dependencies (that point to external repos) to projects within this repo as the dependency would necessarily be at least one commit of forc behind, and as a result it's likely that these dependencies would break at some point in the future, e.g. next time forc makes some breaking change.

@segfault-magnet, @hal3e would you mind testing this branch in your use-case mentioned in #2603 to make sure we've covered the issue?

Edit: I've managed to test this locally by adding some long sleeps immediately after acquiring the lock guard, and then spinning up multiple processes of forc locally to build a toy project with multiple git dependencies. All appears to work fine, with each process blocking and waiting its turn for access to the checkout repo.

This introduces an advisory lock around the re-usable git checkout
repo directories.

Closes #2603, #3484.
@mitchmindtree mitchmindtree added bug Something isn't working forc forc-pkg Everything related to the `forc-pkg` crate. labels Dec 14, 2022
@mitchmindtree mitchmindtree self-assigned this Dec 14, 2022
@mitchmindtree mitchmindtree requested a review from hal3e December 14, 2022 04:05
@mitchmindtree mitchmindtree marked this pull request as ready for review December 14, 2022 04:06
@spiral-ladder
Copy link
Contributor

spiral-ladder commented Dec 14, 2022

The way I was testing it was to fork both sway and fuels-rs, update fuels-rs to build with the concurrent flag and allow manual workflow triggers and use my fork with the lock changes within the fuels-rs GitHub workflow instead. FWIW when I (unsuccessfully) tackled #3484, I tested my fork on my machine which works locally but fails on the CI, so results might differ there.

kayagokalp
kayagokalp previously approved these changes Dec 15, 2022
Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

utACK

@spiral-ladder
Copy link
Contributor

I went ahead and used my personal repo setups to check out your branch and tested it and it works: https://github.com/bingcicle/fuels-rs/actions/runs/3702649777/jobs/6273211503#step:8:236

@mitchmindtree mitchmindtree merged commit efd4518 into master Dec 15, 2022
@mitchmindtree mitchmindtree deleted the mitchmindtree/forc-pkg-checkout-lock branch December 15, 2022 11:45
@hal3e
Copy link
Contributor

hal3e commented Dec 15, 2022

I want to confirm that this works perfectly! Thanks a lot!!! ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working forc forc-pkg Everything related to the `forc-pkg` crate.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running multiple instances of forc concurrently can (very occasionally) cause a git checkout race
5 participants