-
-
Notifications
You must be signed in to change notification settings - Fork 201
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
Massive API breaking change to remove conflicts with MSBuild. #197
Conversation
Previously we used to keep a fork of the same public types and namespaces present in MSBuild (such as BinaryLogger, BinaryLogReplayEventSource, etc). It was easier to sync with the MSBuild codebase. However this presents a problem - if someone is referencing both MSBuild and StructuredLogger.dll then they have a conflict - types with the same namespace and name are present in both places, so it fails the build. I've decided to move public types which are mirrored from MSBuild into a different namespace, and additionally rename BinaryLogReplayEventSource -> BinLogReader. Also switching the version from 1.2 to 2.0 as this is a major breaking change.
Code review please? @AndyGerlicher @rainersigwald @daveaglick @pascalberger @rodrmoya |
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 but I admit to not using this API so I don't have a great "how bad/good is this for users" feel.
Just hoping to catch something really egregious that I'm missing... |
@KirillOsenkov Don't know really the source code, but changes make sense from what I can see. If you wan't I can also build and test it with my source code. |
@KirillOsenkov I tested the changes in my code base and everything seems to work fine |
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.
👍 to moving the types out of Microsoft.Build.Logging
now that MSBuild ships with it's own version. The only thing I can think might break is if someone was using reflection to get one of the types like BinaryLogReplayEventSource
and just swapping the reflected assembly depending on something like the version of MSBuild, but that seems really, really unlikely.
YOLO! Thanks everyone! |
@KirillOsenkov Thank you very much for the fast fix. Really appreciated 👍 |
Previously we used to keep a fork of the same public types and namespaces present in MSBuild (such as BinaryLogger, BinaryLogReplayEventSource, etc). It was easier to sync with the MSBuild codebase.
However this presents a problem - if someone is referencing both MSBuild and StructuredLogger.dll then they have a conflict - types with the same namespace and name are present in both places, so it fails the build.
I've decided to move public types which are mirrored from MSBuild into a different namespace, and additionally rename BinaryLogReplayEventSource -> BinLogReader.
Also switching the version from 1.2 to 2.0 as this is a major breaking change.