Conversation
Forgind
left a comment
There was a problem hiding this comment.
Hello! I am rainersigwald. Tests? 🙂
|
@danmoseley Thanks for a detailed look! Second look would be very appreciated (I attempted to address all resolved comments; the unresolved one might need clarification) |
rainersigwald
left a comment
There was a problem hiding this comment.
This doesn't fully resolve the crash in the repro from #6215 (comment):
❯ dotnet msbuild -nr:false .\error.proj
MSBuild version 17.7.0-dev-23272-01+2b0963cee for .NET
S:\repro\dotnet\msbuild\issues\6215\error.proj(2,20): error MSB4113: Specified condition "''" evaluates to "" instead of a boolean.
MSBUILD : error MSB1025: An internal failure occurred while running MSBuild.
This is an unhandled exception in MSBuild -- PLEASE OPEN A BUG AGAINST THE MSBUILD TEAM.
Microsoft.Build.Exceptions.InvalidProjectFileException->Microsoft.Build.BackEnd.BuildTransferredException: Specified condition "''" evaluates to "" instead of a boolean. S:\repro\dotnet\msbuild\issues\6215\error.proj
at Microsoft.Build.Shared.ProjectErrorUtilities.ThrowInvalidProject(String errorSubCategoryResourceName, IElementLocation elementLocation, String resourceName, Object[] args) in S:\msbuild\src\Shared\ProjectErrorUtilities.cs:line 373
...I think our internal exception types need to be serialized in a type-preserving way.
Thanks for pointing! |
4e8698c to
6a5f6f8
Compare
src/Framework/BuildException/BuildExceptionSerializationHelper.cs
Outdated
Show resolved
Hide resolved
|
cc @ViktorHofer |
|
Thanks @danmoseley and @ericstj for the help! |
|
I did a root cause investigation and shared my finding in dotnet/sdk#32922. Also submitted a fix for it: dotnet/sdk#32930. As we just branched, the fix will likely not be available before Preview 6. While you could use APICompat's nuget package to immediately receive the fix, IMO it's fine to keep this baselined until the repository uses the P6 SDK. I tested my changes on top of your PR and the errors are gone. From that point of view, your changes are correct 👍 |
d7ecbcb to
f12e99a
Compare
rainersigwald
left a comment
There was a problem hiding this comment.
Ok, this isn't a fully complete review but there's enough to ruin your weekend--don't look until Monday morning your time!
src/Build.OM.UnitTests/Microsoft.Build.Engine.OM.UnitTests.csproj
Outdated
Show resolved
Hide resolved
src/Framework/BuildException/BuildExceptionSerializationHelper.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Rainer Sigwald <[email protected]>
b7c6d0b to
4c172dd
Compare
6f4315a to
80e0df6
Compare
Fixes #8786
Context
BinaryFormatSerializer is going to be deprecated (https://aka.ms/binaryformatter) - as a result we should not be using our
SerializeDotNetfunction.This PR is tackling one of the remaining usages - (de)serialization of Exceptions.
Changes Made
InternalLoggerExceptiondoesn have theBuildEventArgsargument preserved during remoting - as it doesn't have straightforward to/from string representations)Unrelated Issues Workarounded Or Addressed
Testing
+ manual test described in the bug
Prior fix:
After fix:
Edit 01:
Edit 02:
Edit 03:
Edit 04:
Edit 05:
Edit 06: