Proper runtime support #2255

matthid · 2017-04-17T12:59:37Z

Part of #2256 but extra PR to make it easier to review.

matthid · 2017-04-17T14:13:41Z

OK this is crazy. If I got that correct some nuget packages can specify additional dependencies for a specific runtime in a file called runtime.json.
This looks like:

  "runtimes": {
    "win": {
      "Microsoft.Win32.Primitives": {
        "runtime.win.Microsoft.Win32.Primitives": "4.3.0"
      },

This means Paket needs to restore runtime.win.Microsoft.Win32.Primitives (which is not known through the regular dependency tree) to support the "win" rid (or any rid importing "win").

How do we do that in paket? I don't want to change the current resolver (as it is complicated enough). For now I'd try to implement this as a second phase (after regular restore has happened). This means that paket will ignore any runtime.json in the second layer (but doing this is insanity IMHO anyway)...

@forki please advise if that sounds like going into the wrong direction...

forki · 2017-04-17T15:43:20Z

Runtime.json is a brain fart. I never understood what people were thinking to implement that hack. Am 17.04.2017 16:13 schrieb "Matthias Dittrich" <[email protected]>:

…

OK this is crazy. If I got that correct some nuget packages can specify additional dependencies for a specific runtime in a file called runtime.json. This looks like: "runtimes": { "win": { "Microsoft.Win32.Primitives": { "runtime.win.Microsoft.Win32.Primitives": "4.3.0" }, This means Paket needs to restore runtime.win.Microsoft.Win32.Primitives (which is not known through the regular dependency tree) to support the "win" rid (or any rid importing "win"). How do we do that in paket? I don't want to change the current resolver (as it is complicated enough). For now I'd try to implement this as a second phase (after regular restore has happened). This means that paket will ignore any runtime.json in the second layer (but doing this is insanity IMHO anyway)... @forki <https://github.com/forki> please advise if that sounds like going into the wrong direction... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNBBzS0ndDDx5WsR1NfVA1arInvt9ks5rw3OVgaJpZM4M_IIr> .

matthid · 2017-04-17T15:49:02Z

And now what?

matthid · 2017-04-17T21:06:16Z

Ok this is an interesting start. I think I'll do the integration in the following way:

Add a new "runtimes" directive to the deps and lockfile

source https://nuget.org/api/v2
runtimes aot,win

nuget Newtonsoft.Json redirects: force

After regular install/update there will be another install/update step for the runtime dependencies for the enabled runtimes

This way restore doesn't need to be changed at all and the new packages will be in the regular resolution (I guess I need a new flag in the lockfile to "detect" if a package is in the regular resolution or if its picked because of runtime).

Still not sure if the default should be installing all packages for all runtimes OR no runtime at all. Currently I'm favoring all runtimes...

cloudRoutine · 2017-04-17T21:14:31Z

I'm not sure that it should default to installing for all runtimes (there are a ton of them) and in the absence of a specified runtime, isn't the default behavior to produce a FDD instead of a SCD ?

cloudRoutine · 2017-04-17T21:21:38Z

src/Paket.Core/Paket.Core.fsproj

@@ -63,6 +63,11 @@
        <FSharpTargetsPath>$(MSBuildProgramFiles32)\Microsoft SDKs\F#\4.0\Framework\v4.0\Microsoft.FSharp.Targets</FSharpTargetsPath>
      </PropertyGroup>
    </When>
+    <When Condition="$(TargetFSharpCoreVersion) &gt;= 4.4.1.0 AND $(TargetFSharpCoreVersion) &lt; 4.4.1.0 ">


These kinds of conditions don't work with xbuild, it's ridiculous that they don't and I'd rather write them this way too, but they end up causing more headaches than they're worth.

Maybe once mono 5.0 drops (replacing xbuild with msbuild 15 by default) this can be considered, though

The target F# core version is a lot less useful when you're actually relying on paket and the fsharp.core nupkg to determine the version you're using

the travis build worked ...

matthid · 2017-04-17T21:35:21Z

I'm not sure that it should default to installing for all runtimes (there are a ton of them) and in the absence of a specified runtime, isn't the default behavior to produce a FDD instead of a SCD ?

Yes, but I'm not sure how they handle native dependencies in that case. From observing I'd say they ignore runtime deps (as in runtime.json) but they still copy stuff in the regular resolution....

cloudRoutine · 2017-04-17T21:37:37Z

src/Paket.Core/RuntimeGraph.fs

@@ -0,0 +1,185 @@
+namespace Paket
+
+open Domain


@matthid can you add some doccoms for the functions in this file? I already know i'll have forgotten what half of this does in a few days

Yes, I added some with c71d62b

cloudRoutine · 2017-04-17T21:45:07Z

src/Paket.Core/RuntimeGraph.fs

+                    RuntimeDependencies = dependencies }
+          | _ -> failwithf "unknwn stuff in runtime-description: %O" t.Value ]
+
+    let readRuntimeGraphJ (json:JObject) =


@matthid do you think it's possible to generalize this json graph structure a bit so we can use it to create the project.assets.json file as well?

can you link an example?

project.assets.json & *.deps.json

No I don't we should generalize here. But of course that info helps in generating those files as well.

forki · 2017-04-18T07:36:21Z

Why do we need runtime in deps file? Shouldn't it just install the runtime we just use?

matthid · 2017-04-18T07:57:13Z

Why do we need runtime in deps file? Shouldn't it just install the runtime we just use?

Good point. But do you really want to read the fsproj file for info and then use that for resolving packages? What about multiple or no fsproj file? csproj? What about "old-style" fsproj? I don't even want to go into that. At least not in the first step (this PR) tbh...

forki · 2017-04-18T07:58:57Z

No not fsproj. I meant the following: install should lock everything, but restore only needs to download the stuff for the target it currently runs on Am 18.04.2017 9:57 vorm. schrieb "Matthias Dittrich" < [email protected]>:

…

Why do we need runtime in deps file? Shouldn't it just install the runtime we just use? Good point. But do you really want to read the fsproj file for info and then use that for resolving packages? What about multiple or no fsproj file? csproj? What about "old-style" fsproj? I don't even want to go into that. At least not in the first step (this PR) tbh... — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNKu_EtBNZguqU3z1dOtXdN3NvOOnks5rxGzZgaJpZM4M_IIr> .

matthid · 2017-04-18T08:02:34Z

What is "the target it currently runs on"?

forki · 2017-04-18T08:03:39Z

The OS if the machine

matthid · 2017-04-18T08:05:06Z

And what if I run on windows and want to publish my project for unix?

forki · 2017-04-18T08:06:49Z

Hook in restore into dotnet publish? Am 18.04.2017 10:05 vorm. schrieb "Matthias Dittrich" < [email protected]>:

…

And what if I run on windows and want to publish my project for unix? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNDXTGRqOVFJszMB1nnuBQsKjuzsBks5rxG6ygaJpZM4M_IIr> .

matthid · 2017-04-18T08:10:02Z

So its the same problem you do another resolver step on "restore" to resolve the runtime deps. I think in pakets philosophy we should have already done that at that phase (IMHO) ... But it's up to you

matthid · 2017-04-18T08:12:43Z

I'd prefer the following when we get called by dotnet restore with an unknown runtime:

If in CI/noninteractive mode we fail when a runtime is not in the deps file and we are called
In interactive mode we ask the user if we should update the deps file and do an "install"

forki · 2017-04-18T08:16:09Z

No not resolver. Only restore For resolving (during install/update) we would check everything. Am 18.04.2017 10:12 vorm. schrieb "Matthias Dittrich" < [email protected]>:

…

I'd prefer the following when we get called by dotnet restore with an unknown runtime: - If in CI/noninteractive mode we fail when a runtime is not in the deps file and we are called - In interactive mode we ask the user if we should update the deps file and do an "install" — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNBBZI-zOeeITT8XoXemUczSaenIgks5rxHB8gaJpZM4M_IIr> .

matthid · 2017-04-18T08:24:40Z

@forki So we always resolve and lock every runtime down (during install/update). I'm fine with that, lets see how that works.
The lockfile might grow quite a bit with this.

But isn't install/update currently downloading the packages as well? Why would we change the behavior for the runtime deps?

I agree that our hook can have this behavior to only download whats needed.

It is really confusing, and I don't know if there is a good way how this fits into our world view...
On the other side I don't expect this PR to be perfect, just a start to experiment how things might work...

forki · 2017-04-18T08:39:26Z

It does not fit our world view. That's the whole point.
It's aspnet team rushing a stupid concept into DNX without telling anyone. Now whole ecosystem is stuck with this shit and nuget team is busy building verified accounts for Microsoft and Oracle but nobody else.

matthid · 2017-04-18T12:47:34Z

src/Paket.Core/AssemblyInfo.fs

-[<assembly: AssemblyInformationalVersionAttribute("4.4.0")>]
+[<assembly: AssemblyVersionAttribute("4.4.1")>]
+[<assembly: AssemblyFileVersionAttribute("4.4.1")>]
+[<assembly: AssemblyInformationalVersionAttribute("4.4.1")>]


@forki one suggestion across all your projects. Can you commit this immediately when you release (at the same time when you change release notes)? This way this doesn't always pop-up and makes merge conflicts.
Another way would be to gitignore those files and having only a default version (like 0.0.0.0) checked in for Visual Studio users...

matthid · 2017-04-18T12:48:59Z

src/Paket.Core/RuntimeGraph.fs

+            | [ a; b ] -> k, f a b
+            | [ a ] -> k, a
+            | _ -> failwithf "This should never happen")
+        |> Map.ofSeq


Is there really no build-in function for that?

fsharp/fslang-suggestions#560

@0x53A thanks :)

…eDependency: true'

matthid · 2017-04-18T17:23:40Z

src/Paket.Core/Files/LockFile.fs

+                    assert (optionsString = "isRuntimeDependency: true")
+                    true, ""
+                else false, optionsString
+            parts.[0],isRuntimeDependency,InstallSettings.Parse(optionsString)


Maybe the parser and generator code should be cleaned up a bit in the future...

…on it...

…uns the integration test suite

add rid and graph parameters

4de3516

matthid added 6 commits April 17, 2017 19:19

add parsing of the runtime graph

b99bc0b

implement merge

e916e0d

add newtonsoft json dep to unit test project

25e4c4a

use vs2017 image for F# 4.1 support

2db9f1f

use F#4.1 for paket.core

51a4768

commit what I have for now

deb17ce

matthid requested review from cloudRoutine and forki April 17, 2017 21:08

cloudRoutine reviewed Apr 17, 2017

View reviewed changes

matthid added 2 commits April 18, 2017 13:28

fix build of test projects

320151f

fix unit tests

8f0435a

matthid commented Apr 18, 2017

View reviewed changes

matthid added 9 commits April 18, 2017 15:28

fix more real-life tests

244cb1a

add comment

b5a96a2

Merge branch 'fix_dotnetcore' into add_runtime_support

981e0e3

add comments to RuntimeGraph

c71d62b

make local nuget feeds work as well.

1af9f33

some cleanup

90aada1

update netcore project file :/

db0fe01

add some tests for runtime dependency resolution.

80dbd1b

add runtime dependencies to the lockfile and mark them with 'isRuntim…

88b2430

…eDependency: true'

matthid commented Apr 18, 2017

View reviewed changes

matthid added 2 commits April 18, 2017 19:34

move InstallModel.fs after RuntimeGraph.fs as we will shortly depend …

d717814

…on it...

cleanup InstallModel again and prepare for runtime support...

5b50f8e

matthid added 4 commits April 19, 2017 09:55

write some release notes

dad17ae

more runtime support in installmodel

e81cda9

appveyor, please build a package for me and publish it.

2a4eb54

finish runtime support in installmodel, fix exlude support

35e28f3

matthid changed the title ~~WIP: Proper runtime support~~ Proper runtime support Apr 19, 2017

matthid added 4 commits April 19, 2017 12:42

cleanup and fixup of 'should understand aot in runtimes' test

48515bd

make two builds: one which fast publishes the package and one which r…

9743b21

…uns the integration test suite

tell appveyor that this are nuget packages.

3473281

verbose is a bit too much

71673ce

cloudRoutine merged commit 71673ce into fsprojects:fix_dotnetcore Apr 19, 2017

This was referenced Apr 21, 2017

FAKE Dotnetcore fsprojects/FAKE#1232

Closed

Followup TODO list for FAKE 5 before a real release fsprojects/FAKE#1523

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Proper runtime support #2255

Proper runtime support #2255

matthid commented Apr 17, 2017 •

edited

Loading

matthid commented Apr 17, 2017

forki commented Apr 17, 2017 via email

matthid commented Apr 17, 2017

matthid commented Apr 17, 2017

cloudRoutine commented Apr 17, 2017

cloudRoutine Apr 17, 2017

baronfel Apr 17, 2017

cloudRoutine Apr 17, 2017

matthid Apr 17, 2017

matthid commented Apr 17, 2017

cloudRoutine Apr 17, 2017

matthid Apr 18, 2017

cloudRoutine Apr 17, 2017

matthid Apr 17, 2017

cloudRoutine Apr 17, 2017

matthid Apr 18, 2017

forki commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

forki commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

forki commented Apr 18, 2017

matthid Apr 18, 2017

matthid Apr 18, 2017

0x53A Apr 18, 2017

matthid Apr 18, 2017

matthid Apr 18, 2017

Proper runtime support #2255

Proper runtime support #2255

Conversation

matthid commented Apr 17, 2017 • edited Loading

matthid commented Apr 17, 2017

forki commented Apr 17, 2017 via email

matthid commented Apr 17, 2017

matthid commented Apr 17, 2017

cloudRoutine commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

forki commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

forki commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

matthid commented Apr 18, 2017

forki commented Apr 18, 2017 via email

matthid commented Apr 18, 2017

forki commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthid commented Apr 17, 2017 •

edited

Loading