-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Fixing regression caused by servicing of System.Data.SqlClient package #42868
Conversation
Do we need to take this for 3.1.3 (march)? |
Not sure yet, this hasn't been brought up to tactics yet, so that needs to happen first, which is why I added the NO MERGE label. |
Please send an email (to tactics) ASAP, if we merge this later than tomorrow morning it will jeopardize getting a 3.1.3 build by Monday |
@@ -4,6 +4,9 @@ | |||
<!-- Must be kept in sync with pkg\Microsoft.Windows.Compatibility\Microsoft.Windows.Compatibility.pkgproj --> | |||
<PackageVersion>4.8.1</PackageVersion> | |||
<AssemblyVersion>4.6.1.1</AssemblyVersion> | |||
<!-- Downgrade the Assembly Version to match RTM in order to have System.Data shim to still | |||
typeforward to RTM version instead of servicing version --> |
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.
What if we put this same condition in all oob libs that were the target of shims? Even if they haven’t bumped the version yet in servicing?
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.
We could do that, but that might have unintended impacts, like for example it may affect other OOB packages that do provide netcoreapp assets and that reference the assembly version that we pinned. Another risk I see with adding a bunch of netcoreapp configurations everywhere, is that we will start preferring those for testing during our vertical build (assets that won't ship anywhere), and we will drop coverage of netstandard assets on test runs that do get published in nuget packages.
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.
I see what you mean about leaking elsewhere.
cc: @ericstj @danmosemsft @Pilchie @Anipik @wtgodbe
For our 3.0 release (and 3.1) we made a major change on how we generate our compile and runtime .NET Framework shims (PR #37550). We did this in order to stop using assembly re-writing and instead to use source generation. Back when we did that, we didn't catch one diff which was that our desktop runtime shims would now reference the netcoreapp vertical contracts and their assembly versions, instead of just using 0.0.0.0 as we used to do with assembly re-writing. This caused an issue that if today we service one of our OOB packages and increase the assembly version, and this assembly happens to be referenced by one of our .NET Framework shims, then now the shims will have a hard dependency on that version of the OOB package and won't work with previous versions of the package. The first instance we've seen is now in 3.1.2 when we serviced System.Data.SqlClient (new assembly version is 4.6.1.1) and it is referenced by our System.Data shim. This means that if you need to load the System.Data shim, are running in 3.1.2 runtime, and reference a System.Data.SqlClient package which is older than 4.8.1 you will hit an exception like:
This will happen because the System.Data is trying to load System.Data.SqlClient new version, but the consuming app was referencing an older version of the package (for example: 4.8.0 which only contains 4.6.1.0 assembly)
This fix will address this regression only, by making System.Data shim to reference 4.6.1.0 version again so that package 4.8.0 will work again, however, older versions of the package will still not work (as was the case already since we initially shipped 3.0 and 3.1). There is a much larger list of assemblies that might hit this issue if they get serviced in the future, so after this regression is fixed, we will work in master branch in order to provide a future-proof fix that will work for all cases.