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

Assembly version changes for system.text.json and dependency injection #45944

Merged
merged 2 commits into from
Dec 12, 2020
Merged

Assembly version changes for system.text.json and dependency injection #45944

merged 2 commits into from
Dec 12, 2020

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Dec 11, 2020

  • We can regress assembly version for ME.DI because we ship ref only in the transport package
  • json change was missing the assembly version update for netfx

@ghost
Copy link

ghost commented Dec 11, 2020

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
  • We can regress assembly version for ME.DI because we ship ref only in the transport package
  • json change was missing the assembly version update
Author: Anipik
Assignees: -
Labels:

area-Extensions-DependencyInjection

Milestone: -

@@ -3,5 +3,6 @@
<PropertyGroup>
<StrongNameKeyId>Open</StrongNameKeyId>
<PackageVersion>5.0.1</PackageVersion>
<AssemblyVersion Condition="'$(TargetFramework)' == 'net461'">5.0.0.1</AssemblyVersion>
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to test this (even one-off/locally) to make sure other assembly version attributes aren't missing for some supported TFM?

Copy link
Member

Choose a reason for hiding this comment

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

@ahsonkhan can you clarify what you meant here? NetFramework is the only place where we must change the version (due to GAC). In 5.0 we are avoiding version changes for any assembly that is also part of the shared framework (and compatible TFMs like netstandard) so that we can have servicable ref-packs.

Copy link
Member

@ahsonkhan ahsonkhan Dec 12, 2020

Choose a reason for hiding this comment

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

NetFramework is the only place where we must change the version (due to GAC)

I understand this is needed for .netfx only. The question I was trying to ask is, how do we verify that this change is correct and that we are not missing other assemblies that would need a similar change?

json change was missing the assembly version update for netfx

For example, how did we detect this originally?

Copy link
Member

Choose a reason for hiding this comment

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

@Anipik reviews the changes that are made in servicing to ensure that they have both packaging and assembly changes since this is something unique to servicing. This is part of the "decision" to service the assembly, you can think of it as the change that represents the decision to opt something into shipping. We're aware that representing this decision requires more changes than we'd like and are thinking of ways to streamline that. Ultimately there will always be some change like this that says: increment a version and ship. We'd like to make it much simpler so that it's literally just that and all the conditions and special cases are handled by the build instead of the developer.

@Anipik Anipik merged commit 988fa3b into dotnet:release/5.0 Dec 12, 2020
@Anipik Anipik deleted the assembly branch December 12, 2020 01:30
@ghost ghost locked as resolved and limited conversation to collaborators Jan 11, 2021
@ericstj
Copy link
Member

ericstj commented Feb 18, 2021

@Anipik we use the Microsoft.Extensions.DependencyInjection when building other libraries.

<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection\src\Microsoft.Extensions.DependencyInjection.csproj" />

<ProjectReference Include="$(LibrariesProjectRoot)Microsoft.Extensions.DependencyInjection\src\Microsoft.Extensions.DependencyInjection.csproj" />

I think that @ViktorHofer made it so that these will get the reference assembly. As such, we may still want to version these non-shipping reference assemblies on .NETFramework so that we get the exact match for P2P references and avoid some unnecessary bindingRedirects. I don't think we've actually shipped any packages yet in 5.0 servicing that would observe this, but moving forward it seems like we can be stricter in our policy around versioning.

@ViktorHofer
Copy link
Member

I think that @ViktorHofer made it so that these will get the reference assembly.

Yes, P2Ping a source project will forward the reference assembly to RAR / the compiler.

@ericstj
Copy link
Member

ericstj commented Feb 18, 2021

Yep, so we should fix this up so that the net461 reference assembly gets the version bump even though it doesn't ship. It's good to know that our rules can be simpler.

I think that makes our versioning rules as simple as.

Is the assembly in a package?

  • Yes
    • Is the assembly also in any shared framework?
      • Yes: Version only the NETFramework configurations
      • No: Version all configurations
  • No
    • Don't version

@Anipik do you agree?

@ViktorHofer
Copy link
Member

ViktorHofer commented Feb 18, 2021

In the yes branch, why not always only update the NETFramework configuration?

@Anipik
Copy link
Contributor Author

Anipik commented Feb 18, 2021

@ericstj we are implementing the same policy currently. (Mentioned in the above comment)

@ericstj
Copy link
Member

ericstj commented Feb 18, 2021

@ericstj we are implementing the same policy currently. (Mentioned in the above comment)

Good good. I think we still need to fix this up:

why not always only update the NETFramework configuration?

It's still desirable to version when we can and matches the reccomendations we give to the community. There are still side-by-side loading issues with components that don't use ALC's. It's definitely something we could consider if we think removing that case would simplify.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants