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.

Sign up for GitHub

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

improve penalty system, add a test and fix #2464 #2498

Merged
merged 1 commit into from
Jul 10, 2017
Merged

improve penalty system, add a test and fix #2464 #2498

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 22 additions & 9 deletions src/Paket.Core/Versioning/PlatformMatching.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ open ProviderImplementation.AssemblyReader.Utils.SHA1
open Logging

[<Literal>]
let MaxPenalty = 1000000
let MaxPenalty = 10000000

type ParsedPlatformPath =
{ Name : string
Expand Down Expand Up @@ -49,6 +49,19 @@ let forceExtractPlatforms path =
| None -> failwithf "Extracting platforms from path '%s' failed" path

// TODO: In future work this stuff should be rewritten. This penalty stuff is more random than a proper implementation.
// Penalty: 000000
// ^ Minor adjustments
// ^ Version jump
// ^ Switch between netcore -> full
// ^ Portable profiles
// ^ Unsupported Profiles
// ^ Fallback
let [<Literal>] Penalty_Client = 1
let [<Literal>] Penalty_VersionJump = 10
let [<Literal>] Penalty_Netcore = 100
let [<Literal>] Penalty_Portable = 1000
let [<Literal>] Penalty_UnsupportedProfile = 10000
let [<Literal>] Penalty_Fallback = 100000
let rec getPlatformPenalty =
memoize (fun (targetPlatform:TargetProfile,packagePlatform:TargetProfile) ->
if packagePlatform = targetPlatform then
Expand All @@ -63,21 +76,21 @@ let rec getPlatformPenalty =
// Just check if we are compatible at all and return a high penalty

if packagePlatform.IsSupportedBy targetPlatform then
700
Penalty_UnsupportedProfile
else MaxPenalty
| _ ->
let penalty =
targetPlatform.SupportedPlatforms
|> Seq.map (fun target -> getPlatformPenalty (target, packagePlatform))
|> Seq.append [MaxPenalty]
|> Seq.min
|> fun p -> p + 1
|> fun p -> p + Penalty_VersionJump

match targetPlatform, packagePlatform with
| SinglePlatform (DotNetFramework _), SinglePlatform (DotNetStandard _) -> 200 + penalty
| SinglePlatform (DotNetStandard _), SinglePlatform(DotNetFramework _) -> 200 + penalty
| SinglePlatform _, PortableProfile _ -> 500 + penalty
| PortableProfile _, SinglePlatform _ -> 500 + penalty
| SinglePlatform (DotNetFramework _), SinglePlatform (DotNetStandard _) -> Penalty_Netcore + penalty
| SinglePlatform (DotNetStandard _), SinglePlatform(DotNetFramework _) -> Penalty_Netcore + penalty
| SinglePlatform _, PortableProfile _ -> Penalty_Portable + penalty
| PortableProfile _, SinglePlatform _ -> Penalty_Portable + penalty
| _ -> penalty)

let getFrameworkPenalty (fr1, fr2) =
Expand All @@ -90,12 +103,12 @@ let getPathPenalty =
let handleEmpty () =
match platform with
| SinglePlatform(Native(_)) -> MaxPenalty // an empty path is considered incompatible with native targets
| _ -> 2000 // an empty path is considered compatible with every .NET target, but with a high penalty so explicit paths are preferred
| _ -> Penalty_Fallback // an empty path is considered compatible with every .NET target, but with a high penalty so explicit paths are preferred
match path.Platforms with
| _ when String.IsNullOrWhiteSpace path.Name -> handleEmpty()
| [] -> MaxPenalty // Ignore this path as it contains no platforms, but the folder apparently has a name -> we failed to detect the framework and ignore it
| [ h ] ->
let additionalPen = if path.Name.EndsWith "-client" then 1 else 0
let additionalPen = if path.Name.EndsWith "-client" then Penalty_Client else 0
additionalPen + getPlatformPenalty(platform,SinglePlatform h)
| _ ->
getPlatformPenalty(platform, TargetProfile.FindPortable path.Platforms))
Expand Down
19 changes: 12 additions & 7 deletions tests/Paket.Tests/InstallModel/Penalty/PenaltySpecs.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ module ``Given a target platform`` =
[<Test>]
let ``it should return the right penalty for a compatible platform``() =
getFrameworkPenalty (DotNetFramework FrameworkVersion.V4_5, DotNetFramework FrameworkVersion.V4)
|> shouldEqual 2
|> shouldEqual (Penalty_VersionJump * 2)

[<Test>]
let ``it should return > 1000 for an incompatible platform``() =
Expand Down Expand Up @@ -62,26 +62,26 @@ module ``Given a path`` =
getFrameworkPathPenalty
[ DotNetFramework FrameworkVersion.V4
Silverlight SilverlightVersion.V5 ] path
|> shouldEqual 1
|> shouldEqual (Penalty_VersionJump)

[<Test>]
let ``it should return the correct penalty for compatible .NET Frameworks``() =
let path = forceExtractPlatforms "net20"
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V2 ] path |> shouldEqual 0
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3 ] path |> shouldEqual 1
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3_5 ] path |> shouldEqual 2
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4 ] path |> shouldEqual 3
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3 ] path |> shouldEqual Penalty_VersionJump
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V3_5 ] path |> shouldEqual (Penalty_VersionJump * 2)
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4 ] path |> shouldEqual (Penalty_VersionJump * 3)

module ``Given an empty path`` =
[<Test>]
let ``it should be okay to use from .NET``() =
let path = forceExtractPlatforms ""
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5 ] path |> shouldBeSmallerThan 2000
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5 ] path |> shouldBeSmallerThan MaxPenalty

[<Test>]
let ``it should be okay to use from a portable profile``() =
let path = forceExtractPlatforms ""
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5; Windows WindowsVersion.V8; WindowsPhoneApp WindowsPhoneAppVersion.V8_1 ] path |> shouldBeSmallerThan 2000
getFrameworkPathPenalty [ DotNetFramework FrameworkVersion.V4_5; Windows WindowsVersion.V8; WindowsPhoneApp WindowsPhoneAppVersion.V8_1 ] path |> shouldBeSmallerThan MaxPenalty

module ``Given a list of paths`` =
let paths =
Expand Down Expand Up @@ -136,6 +136,11 @@ module ``General Penalty checks`` =
let ``best match for DotNet Standard 1.0``()=
Paket.PlatformMatching.findBestMatch (["net20"; "net40"; "net45"; "net451"]|> List.map forceExtractPlatforms, SinglePlatform(DotNetStandard(DotNetStandardVersion.V1_0)))
|> shouldEqual (None)

[<Test>]
let ``prefer net40-client over net35``()=
Paket.PlatformMatching.findBestMatch (["net20"; "net35"; "net40-client"]|> List.map forceExtractPlatforms, SinglePlatform(DotNetFramework(FrameworkVersion.V4_5)))
|> shouldEqual (Some (forceExtractPlatforms "net40-client"))


[<Test>]
Expand Down