-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Forcing auto binding redirects on Microsoft.Extensions.Logging.Console.Tests to fix test issue. #78250
Forcing auto binding redirects on Microsoft.Extensions.Logging.Console.Tests to fix test issue. #78250
Conversation
…e.Tests to fix test issue.
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsFixes #77988 cc: @carlossanlop @tarekgh @ViktorHofer @ericstj
|
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.
Thanks @joperezr
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.
Thank you!
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.
This is a scoped change. Wouldn't it make more sense to fix the assembly version mismatch between ref and src?
It is, intentionally. This is a release branch, so the idea was to provide a fix that was as scoped as possible to mitigate risk, and if possible to avoid changes to non-test code. Given System.Text.Json is inbox in .NET Core, as well as the fact that we don't generally bump reference assemblies build version when we patch unless the ref actually had changes, I'm not convinced that fixing the ref version is correct. Also, src version needs to be 7.0.0.1 and can't be rolled back, as this is a desktop assembly that could be installed on the GAC. One alternative that was possible and still safe in release branch, was to set this property to true for all test projects that have a net48 config, but again, I intentionally picked the scoped fix given we are in a release branch. My suggestion is that if we want a different solution in terms of infrastructure (like making tests build against ref, or setting auto binding redirects to true for all test projects) to happen in main first, and then cherry-picked to release if it makes sense. |
@carlossanlop can you merge this? we need to port this fix to main too I guess. |
We should fix the reference to avoid the need for binding redirects. The mismatch here is due to a reference to the non-shipping reference assembly. @joperezr no one is suggesting any version changes to shipping reference assemblies. Of course we don’t do that in servicing. Enabling auto redirects actually ships this bug to customers. We don’t need to do that. |
Do you mean this reference? Line 23 in 43272d6
|
I'm not sure I follow this. I'm only enabling this on one of our test projects. Unless I'm misunderstanding something, if we change the reference from M.E.L.C to S.T.J from 7.0.0.0 to 7.0.0.1, then this package would be broken when people try to use it in 7.0.100 SDK application (since that one has the unserviced S.T.J). |
Oh, I guess I forgot S.T.J ships in a Nuget package so in that case conflict resolution would pick the oob one and it would be fine. If that change is preferred I'm fine, that said it would no longer be a test only change, so it would require tactics approval, and by extent it would take longer to fix the build. |
I want the serviced M.E.L.C which references the serviced S.T.J package have assembly references which are consistent with package references.
Nothing on .NETCore is impacted. We don’t increment the version there. It’s only netfx. We just need to make the reference targets behave more like the packages do (no reference assemblies when consumer is packable and referencing a packable project). Let me see if I can find where to look. As far as this PR is concerned we could merge it to unblock CI but we should fix this before shipping. |
Maybe here? runtime/eng/resolveContract.targets Line 87 in 43272d6
I’d like @ViktorHofer to look and weigh in since he built a lot of this. I think there should be a very targeted fix here to make the projectreference omit the reference assembly Metadata. The condition could either be “both sides are packable” or “reference is packable and the TFM isn’t a NetCoreAppCurrent TFM”. |
Tagging subscribers to this area: @dotnet/runtime-infrastructure Issue DetailsFixes #77988 cc: @carlossanlop @tarekgh @ViktorHofer @ericstj
|
Tagging subscribers to this area: @dotnet/area-infrastructure-libraries Issue DetailsFixes #77988 cc: @carlossanlop @tarekgh @ViktorHofer @ericstj
|
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.
Approving as a targeted fix to unblock CI failures. We want to revert this change when we have a general solution that makes sure that assembly versions are in sync during servicing or by not referencing the reference assembly at all in cases when those aren't shipping to customers.
Merging now.
|
Fixes #77988
cc: @carlossanlop @tarekgh @ViktorHofer @ericstj