[vcpkg] Use named mutex to serialize dll copies #11363

thomasgt · 2020-05-14T22:55:54Z

I'm not sure if this is the best idea, but it does seem to prevent multiple parallel build workers from clobbering a common CMAKE_RUNTIME_OUTPUT_DIRECTORY when copying runtime deps via applocal.ps1.

msftclas · 2020-05-14T22:56:07Z

All CLA requirements met.

JackBoosY · 2020-05-15T03:08:12Z

Hi @thomasgt, please resolve the conflicts.

Thanks.

thomasgt · 2020-05-15T03:26:59Z

Sure, just rebased onto master.

BillyONeal · 2020-05-15T06:22:52Z

scripts/buildsystems/msbuild/applocal.ps1

+    }
+    finally
+    {
+        $mtx.ReleaseMutex() | Out-Null


Technically this needs a null check but I doubt we care...

thomasgt · 2020-05-15T17:49:27Z

I didn't look closely enough at the resolve function. It calls itself recursively. I guess System.Threading.Mutex doesn't mind recursive waits from the same thread, but this wasn't really my intention. I think it'd be more clear if I just wrap the outer call to resolve with the mutex.

thomasgt · 2020-05-15T18:09:46Z

On the other hand, I guess if other scripts make use of the resolve function, maybe it's better to keep this serialization logic inside? The only usages I've found are from port specific deploy functions that are called from within resolve itself. Just let me know what you think and I'll update accordingly.

strega-nil · 2020-05-18T18:04:09Z

@thomasgt It's not documented, afaict, whether System.Threading.Mutex is recursive or not; therefore, would you mind doing a transform on the code where the recursive bit is pulled out into it's own function, and the existing function just takes the mutex and calls this function?

thomasgt · 2020-06-09T02:28:09Z

@strega-nil Done. Let me know if there's anything else I can clean up!

BillyONeal · 2020-06-09T04:10:30Z

scripts/buildsystems/msbuild/applocal.ps1

+    finally
+    {
+        if ($mtx) {
+            $mtx.ReleaseMutex() | Out-Null


We shouldn't ReleaseMutex unless WaitOne succeeded.

BillyONeal · 2020-06-09T04:11:53Z

scripts/buildsystems/msbuild/applocal.ps1

@@ -90,6 +90,24 @@ function resolve([string]$targetBinary) {
    Write-Verbose "Done Resolving $targetBinary."
 }

+function copyRuntimeDependencies([string]$targetBinary) {
+    try {
+        $mtx = New-Object System.Threading.Mutex($false, "VcpkgAppLocalResolve")


We should probably include the current vcpkg path in here somehow so that totally different vcpkgs don't step on each other.

(Before I knew that we recursively used this when building some ports in vcpkg I was less concerned :) )

Alternately: Should we protect deployBinary on a per binary basis rather than the overall resolution step?

Protecting deployBinary might be a better approach, actually. We could scope the Mutex to a combination of deployBinary's $targetBinaryDir and $targetBinaryName arguments? In that case, there would be no need to include the vcpkg path.

JackBoosY · 2020-06-12T02:42:06Z

Please merge the latest changes into this branch.

BillyONeal · 2020-09-05T01:32:25Z

@thomasgt Are you interested in addressing the above CR comments?

thomasgt · 2020-09-05T02:35:24Z

@BillyONeal This slipped my mind... Yes, I'd be happy to tidy this up next week. I'll take the approach of serializing deployBinary as I alluded to in my reply a few months back.

BillyONeal · 2020-09-05T04:29:18Z

Sounds good, thanks :D

thomasgt · 2020-09-12T20:31:34Z

OK, finally tidied this up. I am scoping the mutex to the target directory, which should let simultaneous builds using vcpkg work as normal, unless they are writing to the same target directory, in which case the DLL copies will be serialized.

JackBoosY · 2020-10-27T09:03:56Z

ping @BillyONeal for review this PR.

JackBoosY · 2020-10-27T09:04:05Z

/azp run

azure-pipelines · 2020-10-27T09:04:16Z

Azure Pipelines successfully started running 1 pipeline(s).

BillyONeal · 2020-10-28T00:54:45Z

@JackBoosY Can you mark status:reviewed in the future if you want the "on call" person to take a look? That way others can see too. Thanks!

BillyONeal · 2020-10-28T00:55:32Z

Thanks for your contribution!

Serialize deployBinary on target directory (microsoft#11363)

PhoebeHui assigned JackBoosY May 15, 2020

JackBoosY added the waiting for response label May 15, 2020

thomasgt force-pushed the serialize-dll-copy branch from 40245e8 to cf2802d Compare May 15, 2020 03:25

JackBoosY added needs-feature-review info:reviewed Pull Request changes follow basic guidelines and removed waiting for response labels May 15, 2020

BillyONeal reviewed May 15, 2020

View reviewed changes

PhoebeHui changed the title ~~Use named mutex to serialize dll copies~~ [vcpkg] Use named mutex to serialize dll copies May 15, 2020

strega-nil added waiting for response and removed needs-feature-review info:reviewed Pull Request changes follow basic guidelines labels May 18, 2020

thomasgt force-pushed the serialize-dll-copy branch from 596d44f to 4a95403 Compare June 9, 2020 00:01

JackBoosY requested review from strega-nil and BillyONeal June 9, 2020 02:32

JackBoosY added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed requires:author-response labels Jun 9, 2020

BillyONeal reviewed Jun 9, 2020

View reviewed changes

JackBoosY added the requires:author-response label Jun 9, 2020

thomasgt force-pushed the serialize-dll-copy branch from 08bebfa to c5bfd27 Compare September 11, 2020 21:06

Serialize deployBinary on target directory

5bb2024

thomasgt force-pushed the serialize-dll-copy branch from c5bfd27 to 5bb2024 Compare September 12, 2020 20:10

JackBoosY removed the requires:author-response label Sep 14, 2020

JackBoosY requested a review from BillyONeal September 14, 2020 02:45

BillyONeal merged commit 10771a8 into microsoft:master Oct 28, 2020

Jimmy-Hu added a commit to Jimmy-Hu/vcpkg that referenced this pull request Oct 28, 2020

Merge pull request #381 from microsoft/master

6f27292

Serialize deployBinary on target directory (microsoft#11363)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vcpkg] Use named mutex to serialize dll copies #11363

[vcpkg] Use named mutex to serialize dll copies #11363

thomasgt commented May 14, 2020

msftclas commented May 14, 2020 •

edited

Loading

JackBoosY commented May 15, 2020

thomasgt commented May 15, 2020

BillyONeal May 15, 2020

thomasgt commented May 15, 2020

thomasgt commented May 15, 2020

strega-nil commented May 18, 2020

thomasgt commented Jun 9, 2020

BillyONeal Jun 9, 2020

BillyONeal Jun 9, 2020

BillyONeal Jun 9, 2020

thomasgt Jun 11, 2020

JackBoosY commented Jun 12, 2020

BillyONeal commented Sep 5, 2020

thomasgt commented Sep 5, 2020

BillyONeal commented Sep 5, 2020

thomasgt commented Sep 12, 2020

JackBoosY commented Oct 27, 2020

JackBoosY commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

BillyONeal commented Oct 28, 2020

BillyONeal commented Oct 28, 2020

[vcpkg] Use named mutex to serialize dll copies #11363

[vcpkg] Use named mutex to serialize dll copies #11363

Conversation

thomasgt commented May 14, 2020

msftclas commented May 14, 2020 • edited Loading

JackBoosY commented May 15, 2020

thomasgt commented May 15, 2020

BillyONeal May 15, 2020

Choose a reason for hiding this comment

thomasgt commented May 15, 2020

thomasgt commented May 15, 2020

strega-nil commented May 18, 2020

thomasgt commented Jun 9, 2020

BillyONeal Jun 9, 2020

Choose a reason for hiding this comment

BillyONeal Jun 9, 2020

Choose a reason for hiding this comment

BillyONeal Jun 9, 2020

Choose a reason for hiding this comment

thomasgt Jun 11, 2020

Choose a reason for hiding this comment

JackBoosY commented Jun 12, 2020

BillyONeal commented Sep 5, 2020

thomasgt commented Sep 5, 2020

BillyONeal commented Sep 5, 2020

thomasgt commented Sep 12, 2020

JackBoosY commented Oct 27, 2020

JackBoosY commented Oct 27, 2020

azure-pipelines bot commented Oct 27, 2020

BillyONeal commented Oct 28, 2020

BillyONeal commented Oct 28, 2020

msftclas commented May 14, 2020 •

edited

Loading