Skip to content

Multi-target all shared framework projects with netcoreapp3.1 #17998

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

Merged
merged 2 commits into from
Jan 15, 2020

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Dec 22, 2019

Without this change, we are still compiling against implementation assemblies in some cases. Based on our current understanding, the problem affects assemblies which are included in the shared framework but are built only for netstandard2.0 or netstandard2.1 TFMs. Due to our current logic, we only compile against ref assemblies which are built for the netcoreapp3.1 TFM. This PR resolves the issue by multi-targeting the affected projects.

Customer impact

The mix of assembly versions in dependencies leads to assembly load errors at runtime in some cases. See aspnet/Announcements#401 for details.

This is a complement to #17970. The two changes together form a comprehensive fix and make everything consistent.

Regression

No. Problem affects 'release/3.0' as well but probably does not meet the bar for 3.0.3.

Testing

Confirmed Windows x86 and OSX artifacts built in https://dev.azure.com/dnceng/public/_build/results?buildId=478589 use consistent assembly versions in the shared framework.

Risk

Minimal. Testing is conclusive and changes are very focused on the specific issue. This PR primarily changes four src/ projects then reacts to that in the generated ref/ projects.

@Tratcher
Copy link
Member

Http.Features was already doing this but was still affected?

@JunTaoLuo
Copy link
Contributor Author

That one is a little more special since it involves a problem with Identity.UI which invokes razor compile in a non-standard method without our custom ref assembly substitution logic and therefore resolves the implementation assemblies. There are quite a few oddities with our builds in this area and I'd like to see this cleaned up a little more due to how fragile it is.

@Pilchie Pilchie added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Dec 23, 2019
@JunTaoLuo JunTaoLuo requested a review from a team December 26, 2019 23:14
@@ -2,7 +2,7 @@

<PropertyGroup>
<Description>ASP.NET Core utilities for key derivation.</Description>
<TargetFrameworks>netstandard2.0;netcoreapp2.0</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netcoreapp2.0;netcoreapp3.0</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to remove netcoreapp2.0 from this? I don't remember the context: Why target the older TFM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would that be considered a breaking change? I don't know if that would be able to make a change such as modifying a TFM in servicing but we have said that adding an additional TFM is allowed in the past.

Copy link
Contributor

Choose a reason for hiding this comment

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

I unfortunatley agree removing 'netcoreapp2.0' would be considered a breaking change. But, I'd still like to understand why the project targets the older TFM in the first place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My guess would be that we just overlooked it? We don't work with this package often. It is strange that we didn't automatically update the TFM to the latest though.

Maybe we should follow up in 5.0 to remove the previous TFM.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. Please file a follow-up issue or open a PR proactively.

@@ -2,7 +2,7 @@

<PropertyGroup>
<Description>ASP.NET Core utilities for key derivation.</Description>
<TargetFrameworks>netstandard2.0;netcoreapp2.0</TargetFrameworks>
<TargetFrameworks>netstandard2.0;netcoreapp2.0;netcoreapp3.0</TargetFrameworks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds reasonable. Please file a follow-up issue or open a PR proactively.

@JunTaoLuo JunTaoLuo added the Servicing-consider Shiproom approval is required for the issue label Jan 9, 2020
@JunTaoLuo JunTaoLuo added this to the 3.1.x milestone Jan 9, 2020
@JunTaoLuo JunTaoLuo changed the base branch from release/3.0 to release/3.1 January 10, 2020 02:12
@dougbu
Copy link
Contributor

dougbu commented Jan 15, 2020

@Pilchie this PR is ready to go other than the servicing-consider label. Has it been discussed in Tactics?

@Pilchie
Copy link
Member

Pilchie commented Jan 15, 2020

It hasn't. We should probably send mail. Also, reconcile it with the guidance in the Versioning spec I mailed yesterday to ensure that things look right.

@dougbu dougbu changed the title Multi-target all shared framework projects with netcoreapp3.0 Multi-target all shared framework projects with netcoreapp3.1 Jan 15, 2020
@dougbu
Copy link
Contributor

dougbu commented Jan 15, 2020

@JunTaoLuo stick w/ the last two branding changes. I'll add the ask-mode template to this PR and forward to Tactics. Will keep you in the loop.

@dougbu
Copy link
Contributor

dougbu commented Jan 15, 2020

W.r.t. the versioning doc, we have not made changes here or elsewhere to address

  1. "packages should reference the lowest version of a package dependency, not the current patch"
    • this is true for netcoreapp3.1 assembly versions but not package versions
  2. "we SHOULD make the assembly version of the implementation the same as the reference assembly and keep it frozen"
  3. It's not clear to me from the doc whether ref/ assemblies need to be in the packages we ship containing shared framework assemblies. If these are needed, we don't do it today.

For what we ship as packages:

These assets MUST reference the assembly version of the reference assembly in the shared framework

This is true w/ this PR but is specific to netcoreapp3.1. .NET Standard assemblies compile against implementation assemblies.

@dougbu
Copy link
Contributor

dougbu commented Jan 15, 2020

🆙📅 to include ask-mode template. Email sent to Tactics.

@dougbu dougbu added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 15, 2020
@dougbu dougbu modified the milestones: 3.1.x, 3.1.2 Jan 15, 2020
@dougbu dougbu merged commit 9522e1d into release/3.1 Jan 15, 2020
@dougbu dougbu deleted the johluo/multitarget branch January 15, 2020 20:08
@dougbu
Copy link
Contributor

dougbu commented Jan 15, 2020

Thanks @JunTaoLuo

@Pilchie
Copy link
Member

Pilchie commented Jan 15, 2020

From above:

  1. We should close on whether we want to do this for 3.1.2 or not soon.
  2. For 2, I think we'll have to switch to that for .NET 5, and continue shipping updated assembly versions for implementation assemblies through 3.x servicing, or else our versions will go backward.
  3. I think that if the ref and impl version of the shared framework asset have different versions (our current state), then we do need to ship the ref and impl in the package, along with netstandard and net4x assets.

Now that we have the doc, we should probably sit down and enumerate exactly where we don't follow it and make a plan for whether/when/how to address violations, and add more clarifications to the doc.

@ericstj @nguerrera - does the above sound right to you?

dougbu added a commit that referenced this pull request Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants