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

[release/9.0] Enable Central Package Management #34666

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Conversation

AndriySvyryd
Copy link
Member

@AndriySvyryd AndriySvyryd commented Sep 12, 2024

Fixes #34638
Fixes #34649

Also ports #34470 and #34576

Related future work:

Description

Due to a recent change in NuGet restore now shows CVE warnings for transitive packages even if the actual version that will be used will be the current one provided by SDK. CPM ensures that all transitive dependencies are pinned to a warning-free version.

Customer impact

Warnings on restore when EF packages are referenced.

How found

Customer reported.

Regression

No

Testing

Tested manually.

Risk

Low.

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

To mitigate product risk on a change like this, can you please share a diff of the nuspecs?

The way I normally do this is as follows:

git checkout hash-preceedingchange
build -pack /p:NuSpecOutputPath=artifacts\specs\beforechange
git checkout changes
git clean -fxd .
build -pack /p:NuSpecOutputPath=artifacts\specs\afterchange

Then I toss the files from before change in a scratch repo, commit, then copy over after change, commit, then share. Here's an example: ericstj/diffs@ee014f1

By diffing the nuspecs you can see exactly what changes in your product dependencies (assuming all you ship is nuget packages). If you ship more than that you might consider other diffs to measure risk.

@AndriySvyryd
Copy link
Member Author

Diff: 280a1f1

@artl93 artl93 self-requested a review September 12, 2024 19:15
Directory.Packages.props Outdated Show resolved Hide resolved
Directory.Packages.props Outdated Show resolved Hide resolved
@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

Diff: 280a1f1

There's a lot changed in this diff - do you understand the differences and do they make sense to you? Which of the packages here ship to customers - will those be OK with the dependency updates in RC2?

I see no effective changes to Microsoft.EntityFramework.Core and Microsoft.EntityFramework.Core.Abstractions which is good, just not sure which of the others to give scrutiny.

Less worried about tests - better to test latest.

@AndriySvyryd
Copy link
Member Author

There's a lot changed in this diff - do you understand the differences and do they make sense to you? Which of the packages here ship to customers - will those be OK with the dependency updates in RC2?

As far as I can see those are all needed to avoid NuGetAudit warnings

Directory.Packages.props Show resolved Hide resolved
benchmark/Directory.Build.props Show resolved Hide resolved
@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

As far as I can see those are all needed to avoid NuGetAudit warnings

I don't think so - the changes for audit were super minimal to product assemblies and only added a couple direct references to tests. The diffs you're seeing are due to applying the versions you used directly in the repo to any where they might occur indirectly. So a single reference in a test might find its way to changing a product depdendency.

Take this one: 280a1f1#diff-3c9435b86bee3d0caf074ebc9ba651f092ea4efcfcd8c6891521486a0a8f24a1R22

You mention in the package list dependencies only used in tests but based on the name of the package Microsoft.EntityFramework.SqlServer I think this is a product assembly. Seems it is getting upgraded from 1.11.4 to 1.12.0. That's what I meant by reviewing the diff - can you please go through that diff and especially for product packages make a comment on whether or not you think the lifted dependency adds risk to deliver in RC2? I can't really make that call as I'm not familiar with EFCore. Like I said if you just focus on the product packages it's probably not too much to do, but better to catch it now then be surprised later.

@AndriySvyryd
Copy link
Member Author

You mention in the package list dependencies only used in tests but based on the name of the package Microsoft.EntityFramework.SqlServer

I'll move these so they only apply to tests

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

I'll move these so they only apply to tests

Ok, but please do comb through an updated diff and make sure you're comfortable with any changes to the product packages.

@AndriySvyryd
Copy link
Member Author

AndriySvyryd commented Sep 12, 2024

@ericstj
Copy link
Member

ericstj commented Sep 12, 2024

@AndriySvyryd -- looking for you and some other EF team member to review that diff and comment on the changes to ensure that everything in shipping packages looks good to go. It's too much change for me to know how to review nor do I know which of your packages are shipping vs not.

@AndriySvyryd
Copy link
Member Author

@AndriySvyryd -- looking for you and some other EF team member to review that diff and comment on the changes to ensure that everything in shipping packages looks good to go

I reviewed them thoroughly and everything looks as expected. Will wait for @roji to give it another pass tomorrow

@roji
Copy link
Member

roji commented Sep 13, 2024

Reviewed the diff, LGTM.,

  • I'm leaving aside test changes (looking at product ones only)
  • Everything in Microsoft.Extensions, System.Text.Json, System.Formats.Asn1 should indeed be updated to the latest 9.0.
  • SQLitePCLRaw.core updating to 2.1.10 is intended, LGTM.
  • Newtonsoft.Json updating to 13.0.3 for Cosmos LGTM.
  • Microsoft.CodeAnalysis and Microsoft.Build.Framework LGTM.

@AndriySvyryd AndriySvyryd merged commit dfb48a1 into release/9.0 Sep 13, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants