-
Notifications
You must be signed in to change notification settings - Fork 525
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
Add net481 support #4227
Add net481 support #4227
Conversation
@@ -919,6 +923,7 @@ type FrameworkIdentifier = | |||
| DotNetFramework FrameworkVersion.V4_7_1 -> [ DotNetFramework FrameworkVersion.V4_7; DotNetStandard DotNetStandardVersion.V2_0 ] | |||
| DotNetFramework FrameworkVersion.V4_7_2 -> [ DotNetFramework FrameworkVersion.V4_7_1 ] | |||
| DotNetFramework FrameworkVersion.V4_8 -> [ DotNetFramework FrameworkVersion.V4_7_2 ] | |||
| DotNetFramework FrameworkVersion.V4_8_1 -> [ DotNetFramework FrameworkVersion.V4_7_2 ] |
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 think there is a missing fallback to 4_8 here.
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.
rather, 4_8 should replace 4_7_2 here
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.
Changed as requested. Tough I do not fully understand, what that functions intent is…
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 not 100% clear, but it is setup like a fallback, and has this comment:
// returns a list of compatible platforms that this platform also supports
I always followed the same pattern of putting the previous version, without skipping or adding more than necessary, I assume some code does a directed graph, and that it allows .net framework v4.8 to be ok with packages that only say v3.5 for example.
Without the adjustment you just made, it may not be possible to reference v4.8 in v4.8.1.
Disclaimer: I've not followed how this is used in the codebase.
@ChrSteinert could you add one test similar to Otherwise, if CI gives similar output as #4219, I think we can move this forward. |
also this one: tests/Paket.Tests/Versioning/PlatformMatchingSpecs.fs |
Add tests for the dependencies-file parser.
Thanks for your guidance and review!
…________________________________
Von: Gauthier Segay ***@***.***>
Gesendet: Thursday, September 28, 2023 7:43:04 AM
An: fsprojects/Paket ***@***.***>
Cc: Christian Steinert ***@***.***>; Mention ***@***.***>
Betreff: Re: [fsprojects/Paket] Add net481 support (PR #4227)
@forki<https://github.com/forki>, I checked the flakytests CI outputs, they look similar to #4219<#4219> AFAICS (beside it pulls dotnet 7 instead of 6).
The added tests cover the support for 4.8.1, I think it is good to go in prerelease mode.
—
Reply to this email directly, view it on GitHub<#4227 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACLJHRKANBXHXLBQETVCCYDX4UE6RANCNFSM6AAAAAA5FZWKA4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
/run merge.and.ship.it |
This aims to add support for net481 framework restriction, and thus resolve #4224 .
The required change seems to be more intricate, than what has changed so far. For instance, there is something about the NuGetPackageCache.CurrentCacheVersion, that I am not sure about, what to do. It also touched a lot of moving parts (and tests) that I adjusted straight forward, and might be missing some things.
Therefore any advice is apreciated.