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

Add net481 support #4227

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Add net481 support #4227

merged 4 commits into from
Nov 14, 2023

Conversation

ChrSteinert
Copy link
Contributor

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.

@@ -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 ]
Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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…

Copy link
Contributor

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.

@smoothdeveloper
Copy link
Contributor

@ChrSteinert could you add one test similar to
tests/Paket.Tests/DependenciesFile/ParserSpecs.fs

Otherwise, if CI gives similar output as #4219, I think we can move this forward.

@smoothdeveloper
Copy link
Contributor

Add tests for the dependencies-file parser.
@ChrSteinert ChrSteinert changed the title [WIP] Add net481 support Add net481 support Sep 25, 2023
@smoothdeveloper
Copy link
Contributor

@forki, I checked the flakytests CI outputs, they look similar to #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.

@ChrSteinert
Copy link
Contributor Author

ChrSteinert commented Sep 28, 2023 via email

@smoothdeveloper
Copy link
Contributor

/run merge.and.ship.it

@forki forki merged commit f0268b4 into fsprojects:master Nov 14, 2023
1 check failed
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.

.Net-Framework 4.8.1 not selectable
3 participants