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

Remove race condition from DllImportGenerator build #61695

Merged
merged 6 commits into from
Nov 18, 2021

Conversation

jkoritzinsky
Copy link
Member

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

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.
@ghost
Copy link

ghost commented Nov 16, 2021

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

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

Author: jkoritzinsky
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@jkoritzinsky
Copy link
Member Author

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

@trylek
Copy link
Member

trylek commented Nov 16, 2021

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

@jkoritzinsky
Copy link
Member Author

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
@jkoritzinsky
Copy link
Member Author

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.

Copy link
Member

@trylek trylek left a 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!

@jkoritzinsky
Copy link
Member Author

There's still another race here apparently (just hit it locally). Back to the drawing board I guess.

@jkoritzinsky
Copy link
Member Author

Got another design. This one should avoid the case that caused a race condition in my last attempt.

@trylek
Copy link
Member

trylek commented Nov 17, 2021

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).

Copy link
Member

@safern safern left a 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 😄

@jkoritzinsky jkoritzinsky merged commit 7f874ee into main Nov 18, 2021
@jkoritzinsky jkoritzinsky deleted the dllimportgenerator-race-fix branch November 18, 2021 02:10
@MichalStrehovsky
Copy link
Member

I just hit a race with what's in main right now. Clean enlistment/deleted artifacts, followed by build.cmd libs -rc Release.

C:\Program Files\dotnet\sdk\6.0.100-rc.2.21505.57\Microsoft.Common.CurrentVersion.targets(3264,5): error MSB3554: Cannot write to the output file "C:\git\runtime2\artifacts\obj\Microsoft.Interop.SourceGeneration\netstandard2.0-Release\Microsoft.Interop.Resources.resources". The process cannot access the file 'C:\git\runtime2\artifacts\obj\Microsoft.Interop.SourceGeneration\netstandard2.0-Release\Microsoft.Interop.Resources.resources' because it is being used by another process. [C:\git\runtime2\src\libraries\System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj]
CSC : error CS1566: Error reading resource 'Microsoft.Interop.Resources.resources' -- 'Could not find file 'C:\git\runtime2\artifacts\obj\Microsoft.Interop.SourceGeneration\netstandard2.0-Release\Microsoft.Interop.Resources.resources'.' [C:\git\runtime2\src\libraries\System.Runtime.InteropServices\gen\Microsoft.Interop.SourceGeneration\Microsoft.Interop.SourceGeneration.csproj]

Rolling back the commit associated with this pull request unblocked me.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 22, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants