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

Update PackageOverrides.txt #104638

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Update PackageOverrides.txt #104638

merged 2 commits into from
Jul 15, 2024

Conversation

ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Jul 9, 2024

Update PackageOverrides.txt which ships inside the Microsoft.NETCore.App.Ref targeting pack with the latest state for .NET 9.

This improves build perf as assembly version checks are avoided for the packages listed with a version lower or equal.

Filed #104639 for adding some automation.

I went through the entire list of inbox libraries from

and checked if there's a package for it and if so, added it to the list or updated its entry. I also updated all the existing entries with the latest package version.

Update PackageOverrides.txt which ships inside the Microsoft.NETCore.App.Ref targeting pack with the latest state for .NET 9.

This improves build perf as assembly version checks are avoided for the packages listed with a version lower or equal.
System.Security.Cryptography.Primitives|4.3.0
System.Security.Cryptography.X509Certificates|4.3.0
System.Security.Cryptography.Xml|4.4.0
Copy link
Member Author

@ViktorHofer ViktorHofer Jul 9, 2024

Choose a reason for hiding this comment

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

From what I can see, this line is wrong as System.Security.Cryptography.Xml was never inbox.

Copy link
Member

@eerhardt eerhardt Jul 9, 2024

Choose a reason for hiding this comment

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

Note that it is "inbox" in Microsoft.AspNetCore.App (which means its not inbox in Microsoft.NETCore.App)

Copy link
Member

Choose a reason for hiding this comment

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

It's in ASP.NET Core but not .NETCore. Surprising that no-one ever hit this, maybe because the package version was old? Or folks listen to us and don't actually use it. I confirmed your analysis in 2.0 - 9.0. Thanks for catching it and removing it.

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 9, 2024

Choose a reason for hiding this comment

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

@eerhardt I see that an entry for it is missing in https://github.com/dotnet/aspnetcore/blob/main/eng/PackageOverrides.txt. There might be others that are missing. Should I file an issue?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like ASP.NET's should be updated to no longer include System.IO.Pipelines, since that is now in NETCore.App.

Copy link
Member Author

Choose a reason for hiding this comment

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

@eerhardt will you take care of updating the PackageOverrides list in aspnetcore or do you want me to file an issue?

Copy link
Member

Choose a reason for hiding this comment

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

I've opened dotnet/aspnetcore#56724 to track updating aspnetcore's file.

@ViktorHofer
Copy link
Member Author

cc @eerhardt

Comment on lines +173 to +174
System.Text.Encodings.Web|9.0.0
System.Text.Json|9.0.0
Copy link
Member

Choose a reason for hiding this comment

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

These 9.0.0 ones are interesting. Is there a reason they weren't included to begin with?

Copy link
Member Author

@ViktorHofer ViktorHofer Jul 9, 2024

Choose a reason for hiding this comment

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

Good question. System.Text.Json is a new package but System.Text.Encodings.Web existed since 2015 and has been inbox since at least netcoreapp2.0. If I'm not mistaken it was you who created the initial list?

Copy link
Member

Choose a reason for hiding this comment

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

This list hasn't changed since you originally added it. I think we were letting platform manifest do the work then.

With PackageOverrides we can short circuit that work. It can also help if we eventually feed this data into NuGet for supplied by platform (short circuiting even earlier in restore).

Copy link
Member

Choose a reason for hiding this comment

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

If I'm not mistaken it was you who created the initial list?

I believe it was initially added in dotnet/core-setup#7369.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

We should consider what's more maintainable for packages that are still built in maintenance-packages or older runtime but not current runtime.

Comment on lines +173 to +174
System.Text.Encodings.Web|9.0.0
System.Text.Json|9.0.0
Copy link
Member

Choose a reason for hiding this comment

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

This list hasn't changed since you originally added it. I think we were letting platform manifest do the work then.

With PackageOverrides we can short circuit that work. It can also help if we eventually feed this data into NuGet for supplied by platform (short circuiting even earlier in restore).

System.Security.Cryptography.Primitives|4.3.0
System.Security.Cryptography.X509Certificates|4.3.0
System.Security.Cryptography.Xml|4.4.0
Copy link
Member

Choose a reason for hiding this comment

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

Update packages that aren't produced from runtime/main anymore but from other branches or from the maintenance-packages repository.
Copy link
Member Author

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

I updated the package versions for ones that will ship again from maintenance-packages or from a servicing branch. Any other feedback?

@ViktorHofer
Copy link
Member Author

@ericstj / @eerhardt any more feedback? If not, would you mind approving the PR?

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. Just some minor questions/comments.

System.Xml.XDocument|4.3.0
System.Xml.XmlDocument|4.3.0
System.Xml.XmlSerializer|4.3.0
System.Xml.XPath|4.3.0
System.Xml.XPath.XDocument|4.3.0
System.Xml.XPath.XDocument|5.0.0
Copy link
Member

Choose a reason for hiding this comment

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

We haven't shipped a new version of System.Xml.XPath.XDocument since 2016. Why does it get 5.0.0 and the other System.Xml packages don't?

Copy link
Member Author

Choose a reason for hiding this comment

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

This package and a few others (i.e. DispatchProxy that you commented on below) will soon get a new version for servicing. The sources got moved to https://github.com/dotnet/maintenance-packages/tree/main/src.

System.Reflection.Emit|4.3.0
System.Reflection.Emit.ILGeneration|4.3.0
System.Reflection.Emit.Lightweight|4.3.0
System.Reflection.DispatchProxy|6.0.0
Copy link
Member

Choose a reason for hiding this comment

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

I think this one can stay at 5.0.0, we never shipped a 5.0.0 (just 5.0.0-preview packages for a while), and it makes it consistent with the other 4.x packages.

@ViktorHofer ViktorHofer merged commit 9600a8c into main Jul 15, 2024
73 of 78 checks passed
@ViktorHofer ViktorHofer deleted the ViktorHofer-patch-2 branch July 15, 2024 15:38
@ericstj
Copy link
Member

ericstj commented Jul 16, 2024

LGTM

@github-actions github-actions bot locked and limited conversation to collaborators Aug 16, 2024
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.

3 participants