search

phmonte / Buildalyzer

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

Handle self-contained apps #194

Closed psfinaki closed 2 years ago

psfinaki commented 2 years ago

As per the docs,

A native executable is required for self-contained deployments.

Therefore this commit broke Buildalyzer for self-contained apps (the usecase is here).

I'm not an MSBuild expert to handle this more elegantly so just reverting the original "optimization" :(

slang25 commented 2 years ago

The change wasn't I initially intended as an optimization, I was getting errors where the apphost wasn't found. Buildalyzer will perform a design time build, which shouldn't be trying to copy native dependencies required for running the app.

Is there a particular issue you are facing?

Edit - ah I see the Stryker .NET issue 👀

psfinaki commented 2 years ago

Yeah there are some real world problems due to this :)

If you know a proper way to set this to "false" only for apps that are not self-contained, I'd be happy to adjust the PR. For me it's a bit of voodoo.

daveaglick commented 2 years ago

If you know a proper way to set this to "false" only for apps that are not self-contained, I'd be happy to adjust the PR. For me it's a bit of voodoo.

This is probably the key. We want to set it to "false" most of the time, except under certain conditions. Ideally we'd be able to grab something like <SelfContained>true</SelfContained> from the project file and base it on that (which isn't fool-proof given directory props files, etc.). So maybe a manual override as well.

Don't have time to take a close look right now, but if @slang25 or someone else wants to figure out a good way to toggle this flag based on the project, that's where I think we need to go.

psfinaki commented 2 years ago

Okay so I'm continuing to look into that... I've figured out that Buildalyzer fails for self-contained apps when the DesignTime is default (= true), see the latest commit. However looks like the tests usually set it explicitly to false.

Also, the original problem is therefore also fixed if we pass this option to the Build method of Buildalyzer.

But I don't completely understand if this is an expected behavior? Is Buildalyzer expected to fail for design-time builds in this case? And what are the risks of universally setting this flag to false then?

daveaglick commented 2 years ago

I'll need to research why some of the tests are turning off design time builds but generally we want that to be the default because it's much faster, though turning it off as an option shouldn't cause any other problems besides a loss of performance (and possibly loosing a little extra diagnostic information which you may or may not be looking for).

Here's more info on design time builds: https://github.com/dotnet/project-system/blob/main/docs/design-time-builds.md.

If turning off design time builds work for this scenario, then that suggests that one of the targets being skipped due to the design time build might make things work - we might be able to light up just that target without totally resorting to a full build, especially if we have some sort of clue that the project is a self contained app. Though if changing the app host setting also works, that's okay too.

Given that we have a set of scenarios that only works when it's false and a different set that only works when it's true, I'd like to figure how to make both work out of the box. If we can't figure out what the value should be before doing a build, maybe we pick the most common, run the build, check the error condition, and then automatically rerun it with a toggled flag if it failed for this reason.

psfinaki commented 2 years ago

Alright, so I pushed some changes... This basically looks if there is "self-contained" element in the project file and doesn't add "UseAppHost" if so. The implementation is trivial but I checked it's enough at least for the original use-case.

slang25 commented 2 years ago

I could be wrong, but I think the way Stryker is using Buildalyzer falls under the DesignTime = false scenario, as you are looking actually build the thing, in a way that it needs to output and run.

Thinking back, another time I've ran into Buildalyzer "clobbering" existing properties is the SolutionDir property that is present in mega-old projects, before NuGet was better integrated into MSBuild, it would be a calculated property. However when doing a design-time build or not, Buildalyzer will try to set it. This creates a ton of work, as I then need to recompute the value and set it, but really I want the MSBuild evaluated version.

I'm thinking rather than baking in lots of smarts around detecting when to set a property, or handing back explicit control over the value, perhaps there should be a configure list of properties you want Buildalyzer to "leave alone". I think this could reduce complexity in the long run, and solve the OP issue. I could remove some workarounds in my projects too.

So if Stryker would continue to use DesignTime = true, the proposal would look like:

AnalyzerManager manager = new AnalyzerManager();
manager.AddPreservedProperty("UseAppHost"); // Naming stuff is hard
...

Then any place internally that sets a property would check this list of properties to preserve, and you get the MSBuild evaluated value, otherwise you end up re-implementing the SDKs, or maintaining good approximations.

psfinaki commented 2 years ago

@slang25 Stryker uses default (true) as of now, see this discussion. But I like your thinking in general. I don't believe I'll be able and have time to implement this though...

daveaglick commented 2 years ago

Before I merge this in and we go down further down the toggling AppHost creation path, I'm going to take a quick step back and see if I can figure out why we needed to disable it in the first place. It's clear that toggling that property seems to impact the ability to build certain .NET 6 projects (like those with implicit usings), but what's not clear to me is how Visual Studio can still perform design-time builds of those same projects. I feel like we're missing some newer magic dust and if I can figure out that, then this whole problem solves itself.

Looking at it over my lunch break today so stay tuned...

psfinaki commented 2 years ago

Sure, and enjoy your lunch! :)

daveaglick commented 2 years ago

Okay, I think I've got a handle on this. The underlying issue is that the default design-time builds were running some non-design-time targets, which in turn made parts fail because everything wasn't working well together.

For example, the underlying cause of the failure in #185 which led to setting AppHost to false in #187 was that some non-design-time build targets were running that expected the apphost.exe to be present as a dependency of the default Build target we run, but because we were also setting the DesignTimeBuild MSBuild property to true other targets that are a little smarter about design-time builds weren't producing that artifact. And we didn't see it in other places because the whole self-contained/application host thing is exclusive to exe project types and most of the tests are libraries.

I think the answer is to swap out the targets for design-time builds with a different set of design-time specific targets. This gets a little tricky since the list at https://github.com/dotnet/project-system/blob/main/docs/design-time-builds.md#targets-that-run-during-design-time-builds appears to be exclusive to Visual Studio. In fact, it's not totally clear that "design-time" is actually completely and totally available outside Visual Studio. Here's the full set of targets Visual Studio uses for a design-time build, many of which aren't available through the SDK alone because they're in a special Visual Studio targets file:

image

It does seem like I can get close enough with a subset of those though:

image

Combined with setting the other global properties that indicate a design-time build I think we might have it. Still testing and might need some additional iteration, but getting closer I think. At a minimum I've create some additional integration tests that look at exe projects, self-contained, and other situations that have been failing around this discussion and they're all passing now.

More to come...

daveaglick commented 2 years ago

Okay, that was sort of a dead end, but it got me in the right direction. We still need to run the Build target since we don't have access to all the actual design-time targets as noted above. In other words, we have to "fake" a design-time build by toggling global properties, which is what Buildalyzer already does. I've probably been down this road a few times already over the years 😂.

So now the trick is which global properties to set that successfully builds exe project types (presumably without doing output folder copying including the apphost.exe) while also building self-contained applications. It looks like the answer is ComputeNETCoreBuildOutputFiles. This is entirely undocumented (like a lot of other Buildalyzer stuff), but by setting that to false:

Reasonably sure I've got it this time: image

That includes new test projects for self-contained executables, .NET 6 exe projects, etc.

I'm going to close out this PR in favor of my incoming commit - thanks a ton for the work here @psfinaki! We wouldn't have ended up in the "right" place without your work pushing it forward.

slang25 commented 2 years ago

Thanks @psfinaki & @daveaglick, my original PR was a misstep, I had no idea the VS design-time behaviour was so involved, I just thought it was a few magic properties 😆

@daveaglick what's the VS extension that shows the MSBuild magic there?

psfinaki commented 2 years ago

@daveaglick thanks a bunch for the proper fix! I've verified it locally for the Stryker case and it works there as expected :) Looking for to the release!

daveaglick commented 2 years ago

@psfinaki All tests were green including the external open source project integration tests, so just released and new version should be showing up on NuGet soon.

@slang25 The magic is in the Project System Tools Visual Studio extension. It logs every build including design-time ones and then lets you open them in the MSBuild Log Viewer. I can honestly say that if it weren't for those two tools, there's no way Buildalyzer would exist.

slang25 commented 2 years ago

@daveaglick It might be worth comparing notes with the OmniSharp folk, they are solving roughly the same problem. I'm betting there will be a lot of cross-over in terms of issues reporting in both repos.

https://github.com/OmniSharp/omnisharp-roslyn/blob/b4042a7389a5492ce5a892576a8df91d31488367/src/OmniSharp.MSBuild/ProjectLoader.cs#L29-L71