GAC Resolution fallback introduces unexpected bugs - should be an error instead of a warning

[NickCraver]

So this one could be fun, because I immediately realize it's going to break some people if implemented. I'm here to argue that's a good thing. This was brought up by an issue we hit while upgrading to .Net 4.6. But, it's far from the first time I've been bitten by it.

MSBuild currently falls back to the framework assemblies in the GAC if the targeted framework reference assemblies are not on the machine. You get a warning like this:

C:\Windows\Microsoft.NET\Framework64\v4.0.30319\Microsoft.Common.targets(983, 5): warning MSB3644: The reference assemblies for framework ".NETFramework,Version=v4.5.2" were not found. To resolve this, install the SDK or Targeting Pack for this framework version or retarget your application to a version of the framework for which you have the SDK or Targeting Pack installed. Note that assemblies will be resolved from the Global Assembly Cache (GAC) and will be used in place of reference assemblies. Therefore your assembly may not be correctly targeted for the framework you intend.

That's a warning, not an error - the build continues. I'm arguing this should break the build, not produce downstream unexpected results.

Here's the relevant code where the MSBuild decision of "can't find the target? let's use the GAC instead", in GetPaths() is made. The GAC fallback seems explicitly intentional, as described here - but I think that's bad behavior, especially with the framework core libraries.

The comment in code notes this:

// No reference assembly paths could be found, log a warning as there could be future errors which may be confusing because of this.

An example is the one we hit, there Array.Empty<T> was used in a Roslyn compiler optimization. Roslyn acted correctly here because it was passed a set of .Net 4.6 assemblies (via the GAC path) on our build server - even though MSBuild was told we were targeting Net 4.5.2. The "helpful" fallback really wasn't - it led to downstream runtime breaks.

Semi-related: .Net 4.5.2 is curiously absent from MSBuild altogether: it doesn't have the paths for .Net 4.5.2 cached, and I noticed 4.5.2 isn't in the TargetDotNetFrameworkVersion enum either. I assume that's because it has no compile-time relevance between 4.5.1 and 4.6, but I thought I would note it here for completeness sake.

In my experience (granted, I may have a slanted view), this fallback behavior usually results in a forward fallback, to a newer version of the framework than reference assemblies are installed for and targeting specified. This brings in assumptions of framework features for compilers and tools to make use of that won't be present at runtime.

If anything, assumptions should be made to older versions of the framework than targeted, since those would be far more likely to be successful. But, I don't think MSBuild should do that either. I think it should break the build, with a helpful error message saying where to get and install the reference assemblies. MSBuild already knows where it tried to find the assembly path for that framework moniker, it should relay this information to the user. Note: the existing error message doesn't provide this info either.

What happens today is a range of not loading to slightly different behavior to runtime errors to just crashing the application. That's much worse (in my opinion) than breaking the build and telling the user how to fix it.

From another view: MSBuild isn't compiling what I asked it to. It's subbing in assemblies and causing problems down the line, possibly in the hands of customers.

So here it is:
How about we make failure to find the targeted framework reference assemblies break the build instead of just a warning and silently not compiling what was asked for?

[rossipedia]

Yeah, gonna go ahead and 👍 this

[Niels-V]

Reading the issues I think it would be nice to upgrade this to an error. But as it is a good practice to treat all your warnings as errors on your project, do you miss these kind of messages from MSBuild?

[NickCraver]

@Niels-V I agree it's good practice, however some good practices often aren't practical. For example while we're moving a huge chunk of code from one pipeline to another we'll use an [Obsolete] attribute to flag the old thing as dead. These can encompass huge tasks (for example we had to dig through every place we encode output in Stack Overflow) and can't happen as a single build - they span multiple builds or weeks (and we build 2-20 times a day, in prod). If we treat all warning as errors then it's not possible to build at all. I think since this option is default-off, the majority of developers probably don't use it (true of most defaults).

I think for small projects it's an excellent option, and one I typically use. For larger projects, it's just not going to happen in many cases for many reasons. Using TreatWarningsAsErrors (or /nowarn) is a bit of a nuclear switch because of its global nature. I would happily treat specific warning codes as errors, but that has practical implications as well:

1. I don't think it's an option, the global switch is all I'm aware of.
2. Most raise the question: why aren't these errors already? (this issue, for example)
3. The system isn't really built like that (integer codes we could flag on - there's no registry of codes).

I'm not unsympathetic to the argument for warning as errors, I just disagree on the practical uses of it once a project is large enough and large changes are happening routinely. I think framework libraries are often an exception to the "large projects"...if that makes sense. I also admit maybe our use cases are severely skewed and my views here are just insane. If that's the case, I'm very happy to be schooled - let me have it :) Does most everyone else use this option and we're in the minority?

[DickvdBrink]

@NickCraver, we don't use the options either. It just not feasible for us because of similar grounds as you gave.
We updated a library we heavily use and we got a few hundreds of extra warnings... I think we fixed most of the warnings now but we couldn't fix all of them at once because it required to much work and we had a release schedule... like most company's have xD :)

[aolszowka]

Is it not feasible to mark this warning as an Error? This seems like a perfect usecase for warningsAsErrors https://msdn.microsoft.com/en-us/library/vstudio/bb629394%28v=vs.100%29.aspx

[AndyGerlicher]

The warnings as errors switch sounds like a good workaround, but given that this behavior is most likely incorrect I agree this should be upgraded to an error so that you don't have to explicitly call it an error. I think this will be an easy change to test and verify, but we will need to evaluate is how much of an impact this would have to the entire ecosystem. And an easy way to opt out and get the old behavior.

[rainersigwald]

@aolszowka The TreatWarningsAsErrors switch in the page you linked only applies to compiler warnings, not MSBuild warnings like this one. I think you're looking for something like #68.

[niemyjski]

This seriously just broke some of our customers. We built against .NET 4.5 and all tests passed. However, when the code is run on a machine that doesn't have .NET 4.6 they get an error: MissingMethodException: Method not found: !!0[] System.Array.Empty() This needs to be escalated and not for up-for-grabs.

[AndyGerlicher]

We've discussed this at length in our team standup today. We all agree that this behavior is incorrect and should result in an error. We plan to fix this in the next Visual Studio release. The question is whether or not to include this as part of an Update (i.e. Update 2). A few options we discussed:

1. Add a feature flag to turn this warning into an error.
2. Add a more complex feature of warnings as errors and recommend upgrading this warning.
3. Upgrade this warning to an error.

Option 2 I don't think we will have time to complete by Update 2, so that's probably out. Option 1 seems OK, but likely by the time someone has been affected by this behavior having a blog post that recommends turning it on seems too late. Ultimately the right answer is option 3, but changing the default could affect a large number of people (we don't have a way to know how many). Knowing that it will break a subset of users makes this a tough call. @jaredpar, when this scenario occurs do you have any idea whether the Roslyn optimizations will break just some or all users in this scenario? Or is it hit or miss depending on code? Right now in the interest of compatibility, we're leaning away from including this change in an incremental update (i.e. Update 2), but will make the change for Visual Studio vNext.

[NickCraver]

When you say the next Visual Studio release, does that correspond to a build tools release? A developer with Visual Studio is far more likely to have the reference assemblies than a build server does, so I'd say a great number of people affected by option 3 may not see anything until it hits a build server.

When the Roslyn optimizations are hit by building on a 4.6+ server without the 4.5 reference assemblies, the affected users are every consumer on 4.5 that tries to run a method missing the type the optimization thinks is there (Array.Empty<T>). This happens in things like myStringBuilder.AppendFormat("test"), which has an empty params on the end.

I think this should definitely be in Update 2 and it should break people. I wish we broke instead of hitting this and I'd be a very appreciative developer if the break was there. By not fixing this as soon as possible the message is that releasing new framework releases and compilers that break people is fine out of band, but fixes aren't. I know that's not the intent and we want to see the best solution, but that is the reality of a delay. This fix should go out ASAP, it's biting more and more people upgrading to .Net 4.6 every day.

[niemyjski]

I completely agree with @NickCraver, this needs to ship ASAP (maybe even out of band hotfix) and be in Update 2 and it should BREAK people. If it breaks as part of the build its 1000000 times better than it breaking at runtime on a customers machine...

[akoeplinger]

A developer with Visual Studio is far more likely to have the reference assemblies than a build server does, so I'd say a great number of people affected by option 3 may not see anything until it hits a build server

+1, this is spot on and is going to hit a lot of people on their build servers. Nonetheless this is the right thing to do, the current behavior is horrible and should result in an error. Having it in Update 2 would be best if at all possible.

The error should print a clear explanation of what is going on and how to fix it, preferably with a link to the reference assemblies package that needs to be installed so developers don't need to hunt for the right package (and maybe add a MSBUILD_I_KNOW_THIS_IS_TERRIBLE_BUT_PLEASE_IGNORE_THE_ERROR=1 env variable to temporarily revert to the old behavior if you're too concerned about blocking people).

[sklivvz]

Completely agree with this, I don't see why this was marked as a warning in the first place.

[AndyGerlicher]

I have a PR out for this now and we're testing internally. Unless there's an unforeseen consequence this will most likely ship in Update 2. Thank you everyone for the input on this, it definitely helped us make this decision.

[niemyjski]

Thank you. If there is anything I can do to help, please let me know.

[niemyjski]

Thank you.

[niemyjski]

@NickCraver did this make it into update 2 ctp?

[rainersigwald]

@niemyjski Yes, it's in Update 2 CTP. I'll make tags and a GitHub release to make that a bit more obvious shortly.

Note that the update installer should apply just fine on a machine with only Build Tools 2015 installed.

[akoeplinger]

@rainersigwald there's no mention of it in the release notes: https://blogs.msdn.microsoft.com/visualstudio/2016/02/10/visual-studio-2015-update-2-ctp/

I think the change deserves a special call out given the impact it'll likely have.

[rainersigwald]

We got this mentioned in the release notes for Update 2 RC: https://blogs.msdn.microsoft.com/visualstudio/2016/03/03/visual-studio-2015-update-2-rc/ (good call, @akoeplinger)

People following this bug might also be interested to know that there is now a standalone Build Tools Update 2 RC package (#480) that can be downloaded from http://go.microsoft.com/fwlink/?LinkId=518023. After Update 2 officially releases, you'll be able to install the equivalent package on build servers to pick up this change.