Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force bindingRedirect for System.Resources.Extensions #39386

Merged
merged 1 commit into from
Jul 16, 2020

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Jul 15, 2020

System.Resources.Extensions writes a type-reference into the .resources
file it writes but RAR will never see this when considering conflicts
for bindingRedirects.

Force a bindingRedirect whenever this package is used.

Fixes #39078

System.Resources.Extensions writes a type-reference into the .resources
file it writes but RAR will never see this when considering conflicts
for bindingRedirects.

Force a bindingRedirect whenever this package is used.
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jul 15, 2020

Tagging subscribers to this area: @tarekgh, @buyaa-n, @krwq
Notify danmosemsft if you want to be subscribed.

force a binding redirect ourselves so that we'll always unify to the System.Resources.Extensions
version provided by this package -->
<ItemGroup>
<SuggestedBindingRedirects Include="$(AssemblyName), Culture=neutral, PublicKeyToken=$(PublicKeyToken)" MaxVersion="$(AssemblyVersion)" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to generate this file since it depends on AssemblyVersion. That way we don't need to "remember" to update another place when AssemblyVersion changes.

</PropertyGroup>
<WriteLinesToFile File="$(_packageTargetsFile)" Overwrite="true" Lines="$(_packageTargetsFileContent)" />
<ItemGroup>
<FilesToPackage Include="$(_packageTargetsFile)" TargetPath="build/$(TargetFramework)" TargetFramework="$(TargetFramework)" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: any reason why not to hardcode $(TargetFramework) to net461? I'm afraid of some future change that may change targetframeworks in the future and might make us package an extra targets file where we shouldn't. Plus, you are already hardcoding the net461 on the condition so it doesn't seem that bad to do it here too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually I try to minimally hardcode things. The condition hardcodes TFM once just as you would to condition items that only apply to a single TFM.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Honestly this workaround feels hacky to me, but I understand this is the best way to mitigate this problem for now.

@ericstj
Copy link
Member Author

ericstj commented Jul 15, 2020

Honestly this workaround feels hacky to me, but I understand this is the best way to mitigate this problem for now.

Setting a specific/matching version in resources is no better since there can always be mismatches in a project graph. I considered a facade with fixed assembly version but that violates our serviceability requirements on desktop (OOB assembly with fixed version). I feel like this is the closest representation of what should happen. The only thing I could think of that's better is if RAR would crack the resources to understand references that might exist there. It could examine this type-reference as well as other type-references that might only appear in serialized resource payloads. The cost/benefit of such a solution doesn't seem worth it.

@joperezr
Copy link
Member

Yeah, I agree that probably the ideal solution would be for RAR to detect this automatically and suggest the binding redirects just as all the other redirects get automatically suggested without us having to manually add them, but again I think this is probably the best solution we can craft for now.

@ericstj ericstj merged commit 1acfcc7 into dotnet:master Jul 16, 2020
@EamonHetherton
Copy link

so this fix will come out with .NET 5 preview 7?

@ericstj
Copy link
Member Author

ericstj commented Jul 16, 2020

Preview7 is already done, we can get it in Preview8. You can also just grab a nightly build off https://dnceng.visualstudio.com/public/_packaging?_a=feed&feed=dotnet5 and validate it.

ericstj added a commit to ericstj/runtime that referenced this pull request Jul 16, 2020
System.Resources.Extensions writes a type-reference into the .resources
file it writes but RAR will never see this when considering conflicts
for bindingRedirects.

Force a bindingRedirect whenever this package is used.
@ericstj
Copy link
Member Author

ericstj commented Jul 16, 2020

#39485 ports to preview8

@ericstj ericstj deleted the resources.Extensions-bindingRedirect branch July 16, 2020 23:40
ericstj added a commit that referenced this pull request Jul 20, 2020
System.Resources.Extensions writes a type-reference into the .resources
file it writes but RAR will never see this when considering conflicts
for bindingRedirects.

Force a bindingRedirect whenever this package is used.
@stonstad
Copy link

I don't believe this made it into .NET 4.8. I'm still seeing this behavior.

@tarekgh
Copy link
Member

tarekgh commented Nov 22, 2020

@stonstad which version of the System.Resources.Extensions package you are referencing in your project?

@stonstad
Copy link

@tarekgh The behavior manifests for me w/ System.Resources.Extensions 5.0.0 and 4.7.2. I am now trying 4.6 per a previous comment/suggestion.

@ericstj
Copy link
Member Author

ericstj commented Nov 23, 2020

@stonstad all this change did was suggest the bindingRedirect. You still need to make sure the project has automatic bindingRedirects enabled and that your application architecture will use the config file. In the project where you're seeing this problem can you double check that it is producing a config file and that config file contains the bindingRedirect?

@stonstad
Copy link

stonstad commented Nov 23, 2020 via email

@ericstj
Copy link
Member Author

ericstj commented Nov 23, 2020

@stonstad can you open a new issue with a minimal repro and we can have a look?

Here's my repro, and it has the redirects:
testSRE.zip

<dependentAssembly>
  <assemblyIdentity name="System.Resources.Extensions" publicKeyToken="cc7b13ffcd2ddd51" culture="neutral" />
  <bindingRedirect oldVersion="0.0.0.0-5.0.0.0" newVersion="5.0.0.0" />
</dependentAssembly>

@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants