V2 is different than V3 #2632
Merged
V2 is different than V3 #2632
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Also this change uncovers some bugs on our v3 implementation, which were hidden because it was never actually used. For example if I let it run on FAKE (paket update) with v3 urls: As well as some "Unable to parse framework |
|
The new warning: |
|
I think now we have too many warnings again :) |
…s in the framework detection logic and makes the detection behave more like NuGet.
|
Ok the error message from above is an unrelated bug: #2634 |
|
This is basically ready (maybe some integration tests need to be updated -> CI), however I expect it to degrade performance a bit. |
matthid
commented
Aug 19, 2017
| None | ||
| else Some { Name = path; Platforms = platforms }) | ||
|
|
||
| let extractPlatforms warn path = |
There was a problem hiding this comment.
The general guideline for warning parameter is:
- true is always OK
- false is only OK when either the calling method prints a better warning OR when the calling method has it's own warning parameter
This way warnings should not get lost. I don't know if there is better way to do this.
|
@dmunch just FYI I removed this |
|
what's the status here? |
|
Failing test is spurious, no idea what's going on |
|
OK found the problem: There is no wpa80 apparently, fix is under way. |
- Add net50 to project files ( update baselines) - Add test to ensure KnownTargetProfiles-lists are updated - Fix wpa parsing: Was parsed as wpa80 but that one doesn't exist (at least not in NuGet). wpa81 is the correct one. - Add hack for uap10.1 to keep project files constant for '.NetCore' (I don't know the correct condition to generate anyway)
matthid
commented
Aug 20, 2017
| let tagReader = FSharp.Reflection.FSharpValue.PreComputeUnionTagReader(typeof<'t>) | ||
| let cases = FSharp.Reflection.FSharpType.GetUnionCases(typeof<'t>) | ||
| checkListEx tagReader cases l | ||
| checkList KnownTargetProfiles.DotNetFrameworkVersions |
|
I feel like this is ready. |
|
For me it's fine to remove it. Since the list is generated by a script
contained in a comment on the top in the list it might however be worth
adding the reason why this particular moniker has been removed.
Le dim. 20 août 2017 à 00:53, Matthias Dittrich <[email protected]>
a écrit :
… @dmunch <https://github.com/dmunch> just FYI I removed this monoandroid44w
thing. Is there any actual package using this? Because afaics nuget doesn't
even support it either:
http://nugettoolsdev.azurewebsites.net/4.3.0-preview4/parse-framework?framework=monoandroid44w
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#2632 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEcbXHjn1cA7DYFLkVyMme0OxSVGbUiKks5sZ2d-gaJpZM4O8UtG>
.
|
|
I might remember wrong about the script, can't easily check the code here.
Still would find it good to add the reason why it has been removed
somewhere for future reference
Le dim. 20 août 2017 à 19:34, Daniel Munch <[email protected]> a écrit :
… For me it's fine to remove it. Since the list is generated by a script
contained in a comment on the top in the list it might however be worth
adding the reason why this particular moniker has been removed.
Le dim. 20 août 2017 à 00:53, Matthias Dittrich ***@***.***>
a écrit :
> @dmunch <https://github.com/dmunch> just FYI I removed this
> monoandroid44w thing. Is there any actual package using this? Because
> afaics nuget doesn't even support it either:
> http://nugettoolsdev.azurewebsites.net/4.3.0-preview4/parse-framework?framework=monoandroid44w
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#2632 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEcbXHjn1cA7DYFLkVyMme0OxSVGbUiKks5sZ2d-gaJpZM4O8UtG>
> .
>
|
|
@dmunch technically the string is not a valid tfm. Nuget.exe is not detecting it either. I don't want to break anything, but can you show an actual package using this string and is nuget.exe working with it? |
|
Note: uap10.1 is also detected as unsupported by paket, but IS detected by nuget: |
|
@0x53A I thought I added that with this PR? |
matthid
commented
Aug 20, 2017
| [<Test>] | ||
| let ``Can detect uap10.1``() = | ||
| let p = PlatformMatching.forceExtractPlatforms "UAP10.1" | ||
| p.ToTargetProfile false |> shouldEqual (Some (SinglePlatform (FrameworkIdentifier.UAP UAPVersion.V10_1))) |
There was a problem hiding this comment.
ah, sorry, I just saw the warning in another build log, and searching in github found the log at the top, where it was also missing, but I didn't actually look at the code.
There was a problem hiding this comment.
no problem. This pr "fixes" tfm parsing to be more like nuget, but it makes supporting monoandroid44w a bit cumbersome. That's why I have removed it.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For example consider https://api.nuget.org/v3-flatcontainer/machine.specifications/index.json?semVerLevel=2.0.0 (and V3 API)
it returns "0.10.0-unstable0014". Paket uses this string currently on a V2 API,
Which is not correct as
returns "0.10.0-Unstable0014". And in fact https://www.nuget.org/api/v2/Packages(Id='machine.specifications',Version='0.10.0-Unstable0014') works
The problem with this change is in fact that it degrades performance. As the V3 GetPackageDetails needs to API calls to get the required information, which the V2 API generally is only a single call.