Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
GAC Resolution fallback introduces unexpected bugs - should be an error instead of a warning #173
Comments
NickCraver
referenced this issue
in dotnet/roslyn
Aug 30, 2015
Closed
.Net 4.6 optimizations breaking on 4.5.2 servers #4889
rossipedia
commented
Aug 30, 2015
|
Yeah, gonna go ahead and |
Niels-V
commented
Aug 30, 2015
|
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
commented
Aug 30, 2015
|
@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 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
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? |
davidfowl
referenced this issue
in aspnet/dnx
Aug 30, 2015
Closed
Remove the fallback to GAC behavior #2591
DickvdBrink
commented
Aug 30, 2015
|
@NickCraver, we don't use the options either. It just not feasible for us because of similar grounds as you gave. |
aolszowka
commented
Aug 30, 2015
|
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 |
|
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. |
AndyGerlicher
added
the
up-for-grabs
label
Aug 31, 2015
|
@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. |
This was referenced Sep 9, 2015
niemyjski
commented
Jan 20, 2016
|
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: |
|
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:
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
commented
Jan 21, 2016
|
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 ( 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
commented
Jan 21, 2016
|
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... |
+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 |
sklivvz
commented
Jan 22, 2016
|
Completely agree with this, I don't see why this was marked as a warning in the first place. |
AndyGerlicher
added a commit
to AndyGerlicher/msbuild
that referenced
this issue
Jan 26, 2016
AndyGerlicher
referenced this issue
Jan 26, 2016
Merged
Promote Framework not found (MSB3644) warning to error #450
|
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
commented
Jan 26, 2016
|
Thank you. If there is anything I can do to help, please let me know. |
AndyGerlicher
added a commit
to AndyGerlicher/msbuild
that referenced
this issue
Jan 26, 2016
AndyGerlicher
added a commit
to AndyGerlicher/msbuild
that referenced
this issue
Jan 26, 2016
cdmihai
closed this
in
0b465d6
Feb 1, 2016
niemyjski
commented
Feb 2, 2016
|
Thank you. |
niemyjski
commented
Feb 10, 2016
|
@NickCraver did this make it into update 2 ctp? |
|
@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. |
|
@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. |
|
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. |
NickCraver commentedAug 30, 2015
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:
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:
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
TargetDotNetFrameworkVersionenum 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?