-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Remove race condition from DllImportGenerator build #61695
Conversation
We had a race condition in the DllImportGenerator build due to the workaround implemented for Roslyn 4.0 RC1's new assembly loading scheme. RC2 has a fix that should work to enable us to remove the workaround.
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer Issue DetailsWe had a race condition in the DllImportGenerator build due to the workaround implemented for Roslyn 4.0 RC1's new assembly loading scheme. RC2 has a fix that should work to enable us to remove the workaround. Fixes #61687
|
It looks like the Roslyn bug that I was working around didn't get fixed until after the RC2 release, so this fix as-is depends on the 6.0 RTM SDK update. I'll try to figure out another fix that works in both VS and at the command line, but it might take a bit |
I just checked on the [succeeding] leg I retried in the originally failing run and I clearly see in the log that the DllImportGenerator project gets built twice - perhaps it's a dumb idea but wouldn't it be possible to just somehow deduplicate this at the build script level? WindowsBase -> D:\a\_work\1\s\artifacts\bin\WindowsBase\net7.0-Release\WindowsBase.dll System.Private.CoreLib.Generators -> D:\a\_work\1\s\artifacts\bin\System.Private.CoreLib.Generators\netstandard2.0-Release\System.Private.CoreLib.Generators.dll Microsoft.Interop.SourceGeneration -> D:\a\_work\1\s\artifacts\bin\Microsoft.Interop.SourceGeneration\netstandard2.0-Release\Microsoft.Interop.SourceGeneration.dll DllImportGenerator -> D:\a\_work\1\s\artifacts\bin\DllImportGenerator\netstandard2.0-Release\Microsoft.Interop.DllImportGenerator.dll DllImportGenerator -> D:\a\_work\1\s\artifacts\bin\DllImportGenerator\netstandard2.0-Release\Microsoft.Interop.DllImportGenerator.dll System.Collections.Specialized -> D:\a\_work\1\s\artifacts\bin\System.Collections.Specialized\net7.0-Release\System.Collections.Specialized.dll System.Collections.NonGeneric -> D:\a\_work\1\s\artifacts\bin\System.Collections.NonGeneric\net7.0-Release\System.Collections.NonGeneric.dll |
We could try to deduplicate it, but we'd be relying on "private" MSBuild semantics in the implementation of ProjectReferences. |
… (GetTargetPath doesn't touch any files, so it shouldn't race). We'll fix this with the real solution once we update to the RTM sdk
I was able to hack something together that should work. It's not the cleanest, but since I'm going to remove the workaround next month, we should be fine. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great to my limited understanding, thanks for providing the stopgap fix!
There's still another race here apparently (just hit it locally). Back to the drawing board I guess. |
…n a new per-project folder side-by-side to avoid race conditions.
Got another design. This one should avoid the case that caused a race condition in my last attempt. |
Sounds like a solid design for the temporary workaround, thanks Jeremy; LGTM to the extent of my still limited understanding of Roslyn generators (after all that's why I asked you to take an initial look in the first place). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I'm glad this is just temporary 😄
I just hit a race with what's in main right now. Clean enlistment/deleted artifacts, followed by
Rolling back the commit associated with this pull request unblocked me. |
We had a race condition in the DllImportGenerator build due to the workaround implemented for Roslyn 4.0 RC1's new assembly loading scheme. RC2 has a fix that should work to enable us to remove the workaround.
Fixes #61687