-
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
Work around a JIT failure when running net472 MSBuild on .NET 8 #10175
Conversation
@MarkKharitonov, please give this a try. |
It works. Could you please explain the essence of the fix for my education? I have looked at the PR change and saw the note about inlining, but I did not understand it. Thank you very much. |
The problematic code is behind an |
Ah, of course. Similar to the Makes total sense now. Hopefully your change is merged and deployed soon. |
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
Co-authored-by: AR-May <67507805+AR-May@users.noreply.github.com>
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 am opposed to taking a fix for this unsupported scenario. IMO we should break harder in the known-bad case, not keep it propped up.
I would lean slightly towards keeping this working and adding a hard break post-17. But if you feel strongly about it, I have no issue with closing this. |
Would you be able to submit a Pull Request for my test case repository at https://github.com/MarkKharitonov/test_load_msbuild_project that exemplifies the correct approach? |
Guys, would it be possible to modify my test case at https://github.com/MarkKharitonov/test_load_msbuild_project to demonstrate how a single app can manipulate both SDK and legacy projects? Our enterprise application still uses .NET Framework and even though almost all the projects are SDK style, there are a few (typically using WCF) which are legacy. We have important tooling processing all of the projects and this move from 17.9 to 17.10 breaks it. I do not mind changing our code to process it correctly, but a working example would make it much easier for us. Now I realize everyone is busy and therefore as a compromise, maybe this PR should be merged after all? This will release the pressure and you will be able to provide a working correct example at ease. |
@MarkKharitonov apologies for the delay, busy times indeed. I'll leave it up to the team to decide if they want to take this fix. But even if it is taken, it will only appear in .NET 9 / VS 17.11. As for the right approach, I believe the guidance is straightforward: When you want to manipulate legacy projects, you should use the MSBuild that comes with VS, not the one in .NET SDK. And because VS MSBuild is designed to run on .NET Framework, it must be hosted in a .NET Framework app. |
Could you provide some more concrete example? In my test case, I process 3 kinds of projects:
And the processor itself:
In reality I do not need to support all the 3 kinds of projects - only legacy and SDK style targeting the .NET Framework is required. Given this context, what are my options? What is the correct approach to develop that processor? |
This is not possible.
Sorry, this is what is required today. |
It was working so far. And the change in this PR has zero cost from all the perspectives - maintenance, performance, code quality, etc... It would be very helpful if this change is merged. |
If the tooling targets .NET Framework, then how can such a tool be packed as a dotnet tool?
This seems to be mentioned here - dotnet tools must target .NET Core. |
You could probably play some tricks like having a dotnet tool that only invokes the .NET Framework app it carries as Content, if you really wanted to distribute as a tool. |
PM take on this: we should not encourage in any way unsupported use of the MSBuild tool. This just increases the support matrix/load on the team and encourages users to use the tool in ways that may subtly break at any time. I am fully in support of @rainersigwald's desires to break more explicitly when this torn state is detected. |
There is ton of legacy .NET Framework code. What you are doing by declaring this scenario as unsupported is doom all the tools that manipulate it and which employ the MSBuild API to be written in .NET Framework as well. |
@MarkKharitonov it's one we have to live with, too. I wish we didn't! But this is what we have. |
Fixes #9869
Context
Running .NET Framework MSBuild in a .NET process is unsupported. Apparently, though, until #9446 it was working.
Changes Made
Worked around the regression by calling .NET Framework-only API in a separate non-inlinable method.
Testing
The test project provided by @MarkKharitonov.
Notes
NB: The scenario is still unsupported and there are no guarantees that it will be working in the future.