-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Stop using 2.x packages in InProcessNewShimWebsite testasset #64120
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
Conversation
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.
Pull Request Overview
This PR updates various package versions across the codebase to address component governance (CG) alerts. The changes include updating Microsoft.Build-related packages from version 17.12.36 to 17.12.50, and updating several ASP.NET Core and Microsoft.Extensions packages in a test project.
- Microsoft.Build package family updated to version 17.12.50
- ASP.NET Core packages updated from 2.2.0 to 2.3.0/2.3.6
- Microsoft.Extensions packages updated from 2.2.0 to 8.0.0/8.0.1
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| InProcessNewShimWebSite.csproj | Updates multiple ASP.NET Core and Microsoft.Extensions package references to newer versions |
| Version.Details.xml | Updates Microsoft.Build package versions and commit SHAs to 17.12.50 |
| Version.Details.props | Updates Microsoft.Build package version properties to 17.12.50 |
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.
I suspect this won't work as I think InProcess support was added in 2.2.
This csproj exists to test ANCM compatibility with older TFMs. Is there a way to change it to target the oldest supported TFM that is not 2.3?
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.
All of these packages were discontinued after 2.2, so I'm not sure - the only update we need to make is kestrel.core, so maybe I'll try just updating that one
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.
No, that doesn't work - would it be sufficient to multitarget DefaultNetCoreTargetFramework and CurrentLtsTargetFramework, and remove all the 2.x PackageRefs?
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.
No because we want to target an older TFM to verify back-compat. Those are the same values currently
Lines 52 to 56 in d434929
Ideally, it'd target net8.0 right now.
Going to separate this into 2 PRs
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.
NetMinimum is net8.0 currently, and is controlled by Arcade
- ASP0019 recommends not calling an API we need to test - ASP0016 is a false positive #64173
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.
Should we revert this and suppress the analyzer warnings here too? I think the #if and the #else branches are currently identical.
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.
I'm fine with that - I was gonna trim it down to 1 branch, but I figured it'd be reasonable to keep the 2 in case the older & newer TFMs have different API surface. You want me to push a commit or were you already doing it?
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.
You want me to push a commit or were you already doing it?
No. I haven't started it. And I think you're right that trimming it down to 1 branch makes more sense if that's possible. I think the FORWARDCOMPAT constant was mostly added to avoid build time errors on down-level frameworks that didn't have the new types. Now that net8.0 is the minimum TFM, most if not all of it is probably unnecessary.
Uh oh!
There was an error while loading. Please reload this page.
Fixes a CG alert