Extension point for package manager. #2667

KevinRansom · 2017-03-21T03:31:58Z

Is the package manager extension point from: #2483

Does not contain the package manager.
Builds on coreclr
Requires package manager key to be at least 2 chars. This allows references to be rooted by a drive letter.

…gration

…imilar to c:\ to work on windows.

smoothdeveloper · 2017-03-21T03:40:24Z

tests/fsharpqa/Source/test.lst

@@ -250,6 +250,7 @@ Misc01			EntryPoint
 Misc01			Globalization
 Misc01,NoMT		Import
 Misc01,NoMT		..\..\..\testsprivate\fsharpqa\Source\InteractiveSession\AssemblyLoading
+Misc01,NoMT		InteractiveSession\Paket


this one should point to UnkonwDependencyManager right?

forki · 2017-03-21T09:53:06Z

Where would we need to put the packagemanager dll?

dsyme · 2017-03-21T13:23:35Z

Exciting!

smoothdeveloper · 2017-03-21T13:29:35Z

src/fsharp/DependencyManager.Integration.fs

+            | Some kv -> ReferenceType.RegisteredDependencyManager kv.Value
+    with
+    | e ->
+        errorR(Error(FSComp.SR.packageManagerError(e.Message),m))


Maybe it wouldn't hurt to have the full stack trace?

Yeah adding some stack trace would be helpful

smoothdeveloper · 2017-03-21T13:31:04Z

src/fsharp/DependencyManager.Integration.fs

+            ReferenceType.Library path
+        | _ ->
+            let managers = RegisteredDependencyManagers()
+            match managers |> Seq.tryFind (fun kv -> path.StartsWith(kv.Value.Key + ":" )) with


We need to fix the lookup, didn't had chance to fix that in original PR but we are using a map, so we should lookup on the exact key rather than iteration.

KevinRansom · 2017-03-21T16:21:50Z

@forki, currently, the directory containing the executing FSharp.Compiler.dll, or the BaseDirectory of the AppDomain or LoadContext, I.e the directory containing fsi.exe.

forki · 2017-03-21T17:37:53Z

I think that would not work for Fsi in VS. We can't guess that path from paket install. We should decide to scan in some specific appdata path. Am 21.03.2017 16:21 schrieb "Kevin Ransom (msft)" <notifications@github.com

…

: @forki <https://github.com/forki>, currently, the directory containing the executing FSharp.Compiler.dll, or the BaseDirectory of the AppDomain or LoadContext, I.e the directory containing fsi.exe. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2667 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADgNAX2XkNrVc9AhXQHEsZBj_qkTPazks5rn_kegaJpZM4MjSrr> .

smoothdeveloper · 2017-03-21T18:57:32Z

@forki / @KevinRansom there are few notes on that topic on the RFC: https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1027-fsi-references.md look for "Handler resolution" on it.

forki · 2017-03-26T09:40:08Z

How can we move forward here?

smoothdeveloper · 2017-04-07T21:40:44Z

@KevinRansom, I think we should keep some more iteration on @forki's branch, because having a paket-less version of this branch makes it harder to do real things with it, and we are losing important tests that found real issues along the way.

I might resume the work some time next week to sketch an implementation for the "handler resolution", and I think working on the other branch makes most sense, what about you submitting patches to the other branch instead?

Also, I think it would make sense for FSI to have the paket manager assembly since there is no hard dependency, just relying on paket.exe. The handler resolution also allow to override handler that we would be shipping out of the box.

Any input?

KevinRansom · 2017-04-07T21:45:31Z

By all means, forki’s branch should be where you guys drive this feature. I need this so that I can start work on what it means for us. I would caution you to resolve the handler in as simple a manner as possible. We can add complexity, once we have some solid experience of the limitations of the most simplest scheme on multiple platforms in various scenarios.

smoothdeveloper · 2017-04-07T21:56:09Z

@KevinRansom can you review the https://github.com/fsharp/fslang-design/blob/master/RFCs/FS-1027-fsi-references.md "Handler resolution" section to tell if you think it is too complicated and what you propose instead?

KevinRansom · 2017-04-07T22:33:04Z

I'm not sure I understand it, I got a bit confused between tool resolution and assembly resolution I thought I did then I lost my place, and sort of got confused.

I think that the rules for #r assembly resolution can be quite simple though.

CAVEAT: The current Windows FSI #r behavior needs to be unchanged on Windows, which makes me wonder about what follows. Perhaps a #newloader pragma to turn on this behavior

for an unqualified assembly name or path without a directory specification
E.g.
"System.Runtime.Collections" or "System.Runtime.Collections.dll"

Load it using the standard Loader semantic. I.e relative to the BaseDirectory of the FSI loadContext.
For an absolute or relative path to the assembly Load it using the standard loader semantic relative to the script directory of the currently executing FSX. If there is no executing script then relative to the fsi.exe location. (Send to FSI needs to specify the executing script location, or project location).
Resolutions using paket or nuget specify the full location of the file on disk.
There is no longer any need to distinguish between ref and impl assemblies. paket or nuget will need to ensure that they add the impl assemblies in the script before the ref assemblies.
For resolving the tool, I think loadfrom files in an environment variable perhaps FSHARP_DM that has a semicolon delimited list of the full path to the location perhaps containing assemblies with the name pattern "DependencyManager.dll", if it's not found there then load from base directory. For sure we will ship paket and nuget integration points in the product so they will be there. And I suppose if someone builds one an NPM integration point, so the environment variable will likely almost never be used

Kevin

KevinRansom · 2017-04-07T22:34:34Z

@smoothdeveloper BTW, the reason I hadn't pulled this was I was hoping to build a test that demonstrated that it worked, but got sidetracked by PDB work..

dsyme · 2017-04-13T13:34:08Z

src/fsharp/DependencyManager.Integration.fs

+        errorR(Error(FSComp.SR.packageManagerError(e.Message),m))
+        None
+
+let resolve (packageManager:IDependencyManagerProvider) implicitIncludeDir fileName m packageManagerTextLines =


Could we have /// comments on all new top-level let , type and module please? thanks

dsyme · 2017-04-13T13:35:44Z

I glanced over this, it's looking very good. What's the plan for adding more testing here? There's only one small negative test so far, and the feature PR should really be self-contained w.r.t. testing?

Also is there any testing for the ref/impl split?

dsyme · 2017-04-13T13:36:34Z

src/fsharp/CompileOps.fs

                            let postSources = (!tcConfig).GetAvailableLoadedSources()
                            let sources = if preSources.Length < postSources.Length then postSources.[preSources.Length..] else []

+                            yield! resolveDependencyManagerSources filename


Why is resolveDependencyManagerSources called twice?

dsyme · 2017-04-13T13:37:52Z

src/fsharp/DependencyManager.Integration.fs

+// * to minimize footprint of integration in fsi/CompileOps
+
+/// hardcoded to net461 as we don't have fsi on netcore
+let targetFramework = "net461"


What's this, and does it need to be hard-coded?

this is remnant from hardcoded parameter we need to give in case of paket.

Notice that IDependencyManagerProvider.ResolveDependencies takes a targetFramework parameter, which has been this value so far.

If we don't give a single framework, it makes it difficult for paket to identify which is intended framework.

I think this should come from tooling, similarly that we have an issue of intellisense always working with latest .net framework rather than having an user option to specify it.

dsyme · 2017-06-27T13:56:13Z

@forki Can you update this when you get the chance? Thanks!

forki · 2017-06-27T14:01:28Z

? I had my own pull request. Not sure what Kevin did here

dsyme · 2017-12-01T16:38:13Z

Closing in favour of #4042

forki and others added 4 commits March 17, 2017 14:10

Initial commit of F# compiler integration

ef0473c

Build on coreclr

16c25d7

Merge branch 'dmintegration' of c:\microsoft\visualfsharp into dminte…

5cb5042

…gration

Requires dmkey to be 2 or more characters, this allows rooted paths s…

4c02ef1

…imilar to c:\ to work on windows.

msftclas added the cla-already-signed label Mar 21, 2017

smoothdeveloper reviewed Mar 21, 2017

View reviewed changes

Identify correct test

eb58bec

smoothdeveloper reviewed Mar 21, 2017

View reviewed changes

dsyme reviewed Apr 13, 2017

View reviewed changes

KevinRansom and others added 2 commits April 19, 2017 11:20

Merge branch 'master' into dmintegration

9975d82

add --lib paths to dependency manager search heuristic

cc8df38

dsyme added the Candidate for F# 4.3 label Jul 2, 2017

matthid mentioned this pull request Jul 25, 2017

[WIP]Adding paket package resolution to fsi and VF# editor #2483

Closed

msftgits removed the cla-already-signed label Sep 26, 2017

dsyme closed this Dec 1, 2017

dsyme removed the Candidate for F# SDK version 4.5 label Mar 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extension point for package manager. #2667

Extension point for package manager. #2667

KevinRansom commented Mar 21, 2017

smoothdeveloper Mar 21, 2017

forki commented Mar 21, 2017

dsyme commented Mar 21, 2017

smoothdeveloper Mar 21, 2017

dsyme Apr 13, 2017

smoothdeveloper Mar 21, 2017

KevinRansom commented Mar 21, 2017

forki commented Mar 21, 2017 via email

smoothdeveloper commented Mar 21, 2017

forki commented Mar 26, 2017

smoothdeveloper commented Apr 7, 2017

KevinRansom commented Apr 7, 2017 via email •

edited by dsyme

Loading

smoothdeveloper commented Apr 7, 2017

KevinRansom commented Apr 7, 2017

KevinRansom commented Apr 7, 2017

dsyme Apr 13, 2017

dsyme commented Apr 13, 2017

dsyme Apr 13, 2017

dsyme Apr 13, 2017

smoothdeveloper Apr 13, 2017

dsyme commented Jun 27, 2017

forki commented Jun 27, 2017

dsyme commented Dec 1, 2017

Extension point for package manager. #2667

Extension point for package manager. #2667

Conversation

KevinRansom commented Mar 21, 2017

Choose a reason for hiding this comment

forki commented Mar 21, 2017

dsyme commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinRansom commented Mar 21, 2017

forki commented Mar 21, 2017 via email

smoothdeveloper commented Mar 21, 2017

forki commented Mar 26, 2017

smoothdeveloper commented Apr 7, 2017

KevinRansom commented Apr 7, 2017 via email • edited by dsyme Loading

smoothdeveloper commented Apr 7, 2017

KevinRansom commented Apr 7, 2017

KevinRansom commented Apr 7, 2017

Choose a reason for hiding this comment

dsyme commented Apr 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dsyme commented Jun 27, 2017

forki commented Jun 27, 2017

dsyme commented Dec 1, 2017

KevinRansom commented Apr 7, 2017 via email •

edited by dsyme

Loading