-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Work around MSBuild task ordering bug #74205
Conversation
@@ -60,6 +60,9 @@ | |||
|
|||
<!-- TODO --> | |||
<_SkipUpgradeNetAnalyzersNuGetWarning>true</_SkipUpgradeNetAnalyzersNuGetWarning> | |||
|
|||
<!-- https://github.com/dotnet/msbuild/issues/10306 --> | |||
<CoreCompileDependsOn>$(CoreCompileDependsOn);ResolveKeySource</CoreCompileDependsOn> |
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.
This will solve the immediate issue but I'm worried about other similar target dependencies. No objection to checking it in but I'm not sure it'll fix everything.
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.
I have actually seen some more non deterministic issues while having this fix locally, so we should probably solve differently. Thanks
@jjonescz / @rainersigwald until the msbuild fix comes in from dotnet/msbuild#10306 (comment), can we publish and check this in? |
I'm not 100% sure this won't cause other issues, but if you are observing the bug and can try this out locally to verify it works, then we can merge it. |
restore in VS is also failing:
Causing major failures in VS: |
This branch is not up to date, please just try the change on latest main. Those NU1010 errors were fixed by #74299. |
@jjonescz What would you need to be able to move forward on this. Right now the core issue is preventing being able to dev in VS reasonably :( Thanks very much for your help! |
From the picture it doesn't look like the latest main is merged in, I see #74291 as the last commit in the main you merged if I interpret the picture correctly. That doesn't have the fix from today. I have updated this branch with the latest main now. |
If this workaround doesn't work, let me know, I can look again to see if there is a better workaround or if we can flow the internal fix here sooner. |
The internal fix is getting inserted into VS now via https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/563555. ETA tonight. |
Don't we need the VSSDK packages to be updated in Roslyn? I've tried that in #74329 but tests are failing, it's outside my area of expertise. |
Yes I ve sent the PR to the Extensibility team you validate the version that got published. In the meantime, we would want to work with our infra on test failures. |
I'm not sure if we can easily update VSSDK, see #74329 (comment). But from #74320 (comment) I understand this workaround works for @CyrusNajmabadi, so perhaps we can merge it in the meantime. |
Fixes #74156.