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

[vcpkg] Add x-ignore-lock-failures #14397

Merged
merged 5 commits into from Nov 12, 2020

Conversation

strega-nil
Copy link
Contributor

This should fix #14281 at least as a stopgap.

Additionally, adds better errors.

This should fix microsoft#14281 at least as a stopgap.

Additionally, adds better errors.
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Does this do the right thing? x-wait-for-lock is already off by default...

scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Show resolved Hide resolved
scripts/buildsystems/vcpkg.cmake Outdated Show resolved Hide resolved
toolsrc/src/vcpkg/vcpkgpaths.cpp Outdated Show resolved Hide resolved
@BillyONeal
Copy link
Member

(To clarify, I thought we only set --x-wait-for-lock in the msbuild integration pieces which should be unaffected by this NFS stuff since that's Windows?)

@strega-nil
Copy link
Contributor Author

@BillyONeal yeah, disable-lock allows a user to just deal with "locking a file fails for a reason that isn't someone else owning the lock"

@BillyONeal
Copy link
Member

@BillyONeal yeah, disable-lock allows a user to just deal with "locking a file fails for a reason that isn't someone else owning the lock"

That's not what this does though; it's making disable-lock unconditionally skip everything related to locking

@strega-nil
Copy link
Contributor Author

yeah, exactly; if someone is dealing with "Weird Things", they can use this to disable the lock to just get things working.

@BillyONeal
Copy link
Member

yeah, exactly; if someone is dealing with "Weird Things", they can use this to disable the lock to just get things working.

Why can't they just not pass --wait-for-lock then? That's what I meant by

(To clarify, I thought we only set --x-wait-for-lock in the msbuild integration pieces which should be unaffected by this NFS stuff since that's Windows?)

Can't we distinguish between "failed to take the lock because of an error" and "failed to take the lock because there's another vcpkg alive here"?

@strega-nil
Copy link
Contributor Author

@BillyONeal yes, we can, that's why --wait-for-lock exists. The errors on the original thread were errors that weren't caused by another vcpkg. Are you asking if we should add a --allow-lock-to-fail or something?

@BillyONeal
Copy link
Member

I'm saying that it is unclear here why instead of adding a switch that says "don't do X", we aren't just asking the user to not pass the switch that says "do X". If the user didn't pass --wait-for-lock, why are we treating any management of the lock as a fatal error?

@JackBoosY JackBoosY added category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. labels Nov 5, 2020
@strega-nil
Copy link
Contributor Author

@BillyONeal you need a lock in order to do anything? I'm confused where you're confused.

If you have a vcpkg install going, it needs to access the buildtrees and the like in a mutable way. Therefore, you need mutual exclusion. We do that via the lock file; if --x-wait-for-lock is passed, then we wait until the lock is free, else we try a few times and then print an error if we can't get the lock.

This solves an entirely different issue, which is where the lock isn't get-able for reasons that aren't "it's already taken".

@BillyONeal
Copy link
Member

I discussed this with @strega-nil over IM and my confusion was because I was thinking the world was like this:
image
and so I was wondering why were adding a switch that just undid what another switch did. The actual effects post this PR are:
image

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Nov 6, 2020
@BillyONeal
Copy link
Member

This is "info:reviewed" but I'm not merging it because Nicole has the power to do that herself and might want further feedback from others on the team

toolsrc/include/vcpkg/vcpkgcmdarguments.h Outdated Show resolved Hide resolved
@BillyONeal BillyONeal changed the title [vcpkg] Add disable-lock options [vcpkg] Add x-ignore-lock-failures Nov 12, 2020
@strega-nil strega-nil merged commit 2dc7088 into microsoft:master Nov 12, 2020
@strega-nil strega-nil deleted the cmake-toolchain-manifests branch December 10, 2020 20:36
strega-nil added a commit to strega-nil/vcpkg that referenced this pull request May 5, 2021
* [vcpkg] Add disable-lock options

This should fix microsoft#14281 at least as a stopgap.

Additionally, adds better errors.

* billy CRs

* change from "disable-lock" to "allow-spurious-lock-failures"

* billy cr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:vcpkg-bug The issue is with the vcpkg system (including helper scripts in `scripts/cmake/`) info:internal This PR or Issue was filed by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[vcpkg] Locking .vcpkg-root fails on NFS drives
3 participants