search

phmonte / Buildalyzer

A utility to perform design-time builds of .NET projects without having to think too hard about it.
MIT License
593 stars 94 forks source link

upgrading project and msbuild deps #140

Closed colombod closed 4 years ago

colombod commented 4 years ago

Addressing issue #139

daveaglick commented 4 years ago

It looks like binlog version 9 was introduced in a commit on January 14: https://github.com/microsoft/msbuild/commit/6426e79f1ac49d3740f97ea4722cc13774eb23ba#diff-44a058faa22f024c71a08263069158bcR39

That makes me wonder if 15.8.166 is even new enough to read version 9 logs. There's a balance here - the further forward we move the MSBuild dependency, the more recent our target has to be. The 16.x releases of MSBuild don't target .NET Standard anymore either so if we have to go to them then it's into multi-targeting which I'd really like to avoid. Need to do a little more research on binlog version compat.

daveaglick commented 4 years ago

I was having trouble reproducing any errors at all with binlog reading so I started manually peeping the version flag on binlogs created from the CLI. It turns out I'm only getting version 7 binlogs on .NET Core SDK 3.1.202. I updated to 3.1.300 and sure enough I'm now seeing version 9. So it looks like version 9 is only produced by the latest SDK for .NET Core 3.1.

Reproducing inside a unit test is also tough because the integration tests that read binlog files produce them by triggering binlog production using Buildalyzer, which adds the BinaryLogger from the referenced MSBuild to the post-streamed logging events (as opposed to instrumenting inside MSBuild directly with something like /bl). That guarantees Buildalyzer can always read the binlogs it produces, but might not be able to read other binlogs. I'll need to create a new integration test that generates a binlog from MSBuild and tries to read that in order to verify a fix.

colombod commented 4 years ago

How can I help you on this? That is exactly the repro. How do we make sure that we can keep on taking advantage of Buildalyzer in botnet try? Do you want to have a look over to botnet try integration as well?

daveaglick commented 4 years ago

Not sure, but getting closer. I can now produce a version 9 binlog using the /bl switch inside an integration test. As expected, it fails even with the dependency bump to MSBuild 15.8.166:

image

I think I've got three options:

I'm leaning towards the last approach and doing some experiments. We'll figure out one way or another - I'll let you know when/if I could use an assist.

colombod commented 4 years ago

I love the 3rd option. I'd also propose to bump Buildalyzer up a major version number, this is quite a strong change and it is important to get it as visible as possible to the users.

Let me know how can I help or if you want to pair on this.

daveaglick commented 4 years ago

Success!

image

This is kind of a versioning mess. Turns out the third option won't work - I was actually already using the separate MSBuild.StructuredLogger package to read binlogs, but looking at the release history the latest version it'll go to before bumping it's own deps to the 16.x versions of MSBuild is binlog version 8.

I ended up bumping everything up to the latest but then continuing to target netstandard2.0 and turning off warnings (which I treat as errors) for NU1701 ("this package may not be compatible..."). Buildalyzer builds and runs fine since it doesn't use anything past .NET Standard 2.0 APIs internally and as long as the calling application targets .NET Core 2.1 or .NET Framework 4.7.2 or higher everything will match up and I can avoid multi-targeting Buildalyzer. Of course that's a pretty big target bump for consumers so I'll increment the major version and document it.

I merged your changes in manually and then iterated, so I'll close the PR once the release is out. Just need to do a little more verification, but assuming other integration tests all run I'll get a release out and you can take it for a spin. Let me know if it still doesn't work or something else breaks and we'll keep at it.

colombod commented 4 years ago

Wow amazing, let em know as soon as it is ready osi can take it in and test. Thank you so much

daveaglick commented 4 years ago

It looks good - even the OSS projects pass (at least locally):

image

Publishing 3.0.0 to NuGet right now.

colombod commented 4 years ago

Thank you. I got the package and works! Now onto some more upgrade pain in other areas but then I log issue is gone!

daveaglick commented 4 years ago

Excellent, now I just need to update my own stuff 🤣