-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[main] Update dependencies from dotnet/roslyn #8674
[main] Update dependencies from dotnet/roslyn #8674
Conversation
…417.19 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-1.23217.19
@rainersigwald any idea what is going on here cc @jaredpar |
Adapted to dotnet/roslyn#67747. @lewing any particular reason you were looking at this? |
just trying to make sure there is nothing more hiding in the package flow ahead of preview.4 freeze |
As of dotnet/roslyn#67747, the name is just netcore.
3fe57e3
to
dc5864d
Compare
It amuses me that the package-binary-compat-check task is failing with a binary compat failure, but . . . what? |
@dotnet/roslyn-analysis could someone take a look? |
…422.1 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-1.23222.1
cc @dotnet/dnr-codeflow |
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 fine to me. I believe there is a validation book associated with this update. @mavasani to approve as well.
I can repro the failure when building from 17.5. fuslog:
From a successful binlog it finds
|
@Forgind can you please pick up the investigation since you're kitten? Lower priority than 17.6 and 17.7-preview1 stuff. |
…501.1 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-2.23251.1
…505.2 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-2.23255.2
…512.3 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-2.23262.3
@steveharter noticed this locally and was sharing with @ViktorHofer. I remember a similar version of this problem before In that case it was due to torn state between roslyn assemblies. I wonder if that could be the cause here as well and somehow the preloading that we do didn't help. |
Here's a wild guess. This could be the result of the newly added Also, the use of |
I like the theory but I think it's not right, because the failing leg is "use Visual Studio 17.5 to build the repo in release mode with SDK 7.0.203" which shouldn't be aware of the
This still seems true though :) |
Yeah I just came to the same conclusion. Based on the binlog, the correct path is passed in to APICompat: My current guess is that before, the events were firing and resolving the assemblies from the custom path because the Task assembly had an assembly reference to the CodeAnalysis assemblies. With some refactoring this could have changed a few months ago so that its not ApiCompat.Task.dll that triggers the load of the Microsoft.CodeAnalysis assemblies but ApiCompatibility.dll which doesn't have the RoslynResolver enabled. |
I think I understand this now. The problem is binding redirects on Specifically, However, Roslyn 4.7 has moved to SCI 7.0.0.0 (as has MSBuild 17.6 but that's not relevant here because that AzDO machine definition is still on 17.6). That isn't automatically unified by the binding redirect (it's out of the range
However, the task references Then the failure is
The mismatch is on I think this is a problem that a user could run into for certain combinations of VS and SDK, but I don't know if it's a bug worth fixing, especially since VS 17.5 was STS and is now out of support. Our repo won't notice this problem in a couple of weeks after the build image is updated to VS 17.6, when MSBuild will binding redirect everything to 7 and unify again. |
Makes sense. It’s a type agreement problem. We roll forward to latest Roslyn but don’t do so for its dependencies which overlap with ours (and the types we exchange with Roslyn). I wonder if we should add more assemblies to the set that gets loaded from Roslyn path? |
I think you might have to spin up an AppDomain to get that fully right, since MSBuild also uses SCI (but not on our public interface so having your own in the task that matches Roslyn would be fine). The loader was perfectly happy finding SCI 7 for Roslyn in the LoadFrom context, but it didn't do that when the task itself asked for SCI 5 since that was already satisfied. |
@MichalPavlik to unblock this codeflow, I think we could switch the |
We already do some manual loading. We've got two host/plugin releationships here. MSBuild is hosting the task and needs to unify all the types it talks to the task with. Luckily MSBuild controls this pretty well with a bindingRedirect hammer for it's own types and the framework unification for framework types (and is careful not to add more types to those interfaces). This task is the "host" for roslyn but we didn't take care to unify all the assemblies in Roslyn's public API -- only Roslyn itself. So we could look at all the assemblies that make up the public API and try to load those from the roslyn path. |
…521.1 Microsoft.Net.Compilers.Toolset From Version 4.6.0-2.23171.5 -> To Version 4.7.0-2.23271.1
Doing this in #8789. |
Move (only) the `Windows Full Release (no bootstrap)` leg to the `windows.vs2022preview.amd64.open` pool, which has Visual Studio 17.6 on it already. This will unblock #8674 by avoiding dotnet/sdk#32691; the `MSBuild.exe.config` binding redirects in the 17.6 VS will keep the current versions working for the moment.
This pull request updates the following dependencies
From https://github.com/dotnet/roslyn