[WIP] - Build packagemanagement based on Steffen's Package manager design. #4042

KevinRansom · 2017-12-01T01:17:51Z

This is @forki's original PR. With the paket specific stuff removed.

Discussion: #2483

So there will be a few minor tweaks to implement a package based extensibility mechanism, that will be aligned with Don's proposal for TypeProvider design time components. Other than that the basic idea remains unchanged. The package manger receives the text for a #r" it ensures that the packages are deployed, provides FSI a script fragment that references the correct dlls to be executed.

Packages can be specified and loaded during an interactive session.
new PackageManagers can be loaded whilst a sript is executing and from that point on in the session.

Todo:

Implement compilerTools extensibility mechanism in compiler.
Rationalize extensibility probing to match Type Provider design time mechanism from: Type provider design time component loading suitable for .NET Core tooling #3864
Unit tests for compilerTools extensibility
Unit tests for packagemanager extension point
A default package manager for windows desktop / Cross Platform coreclr

…to forki-paket

…harp into forki-paket

saul · 2017-12-01T09:54:09Z

src/fsharp/FSComp.txt

@@ -1422,3 +1422,6 @@ notAFunctionButMaybeIndexer,"This expression is not a function and cannot be app
 notAFunctionButMaybeDeclaration,"This value is not a function and cannot be applied. Did you forget to terminate a declaration?"
 3218,ArgumentsInSigAndImplMismatch,"The argument names in the signature '%s' and implementation '%s' do not match. The argument name from the signature file will be used. This may cause problems when debugging or profiling."
 3219,pickleUnexpectedNonZero,"An error occurred while reading the F# metadata of assembly '%s'. A reserved construct was utilized. You may need to upgrade your F# compiler or use an earlier version of the assembly that doesn't make use of a specific construct."
+3220,couldNotLoadDependencyManagerExtenstion,"The dependency manager extension %s could not be loaded. Message: %s"


%s should be in single quotes here

saul · 2017-12-01T09:55:42Z

src/fsharp/DependencyManager.Integration.fs

+        try
+            CustomAttributeExtensions.GetCustomAttributes(theAssembly)
+            |> Seq.tryFind (fun a -> a.GetType().Name = attributeName)
+            |> function | Some _ -> true | _ -> false


This can be replaced with: |> Seq.exists (fun a -> ...

saul · 2017-12-01T09:57:42Z

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 is this actually used for? Can we not determine our current .Net Framework version instead of hardcoding?

eventually yes. but not needed now

Well what if I’m on .Net 4.5?

then fsi is still...

Right but this is going to be passed to nuget/paket/whatever to determine which assembly to reference... the precise .Net Framework version is important.

Hard coding net461 is wrong for FSharp.Compiler.Service.dll (which can be used to build tools that run on other frameworks) and for editing environments. I'd rather see this passed down from the host tool, or otherwise configurable somehow.

Of course hard coding is wrong, but it's a bit tricky since it needs to be the executing environment. Not necessarily the fcs environment. Consider we host the feature in fake

Then ionide might run in net461 but fake process in netcore. Of course fake would restore a second time, but that would be a waste

Then ionide might run in net461 but fake process in netcore. Of course fake would restore a second time, but that would be a waste

I see.

It is true that the "default assumed target framework" for the F# scripting model is net461. That is, an editing tool, or execution tool should process a script on the assumption it is targeting net461.

For FSI on .NET Core this will be netcoreapp20 (we should at least define this conditionally here?), but editing tools will still have to assume net461 by default.

Because of this, we will likely need to add a directive #framework "net451" or #framework "netcoreapp20" I suppose, to allow editing tools to know that the script is really designed for use on .NET Core.

Specification and switching of an alternate target framework is TBD. The default target framework will either depend on the host or be netstandard2.0.

saul · 2017-12-01T09:59:10Z

src/fsharp/fsi/fsi.fs

+                            tcConfigB.AddIncludePath(m,folder,"")
+
+                        match loadScript with
+                        | Some loadScript -> istate := fsiDynamicCompiler.EvalSourceFiles (ctok, !istate, m, [loadScript], lexResourceManager, errorLogger)


Instead of using a ref variable here, this can be restructured as a Seq.fold over tcConfigB.packageManagerLines

realvictorprm · 2017-12-01T11:48:37Z

Thank you @KevinRansom! :)

dsyme · 2017-12-01T13:38:01Z

src/fsharp/CompileOps.fs

+                        | _ ->
+                            match DependencyManagerIntegration.tryFindDependencyManagerByKey m packageManagerKey with
+                            | None ->
+                                errorR(DependencyManagerIntegration.createPackageManagerUnknownError packageManagerKey m)


Perhaps this should just be a warning. There will be a later error almost for sure - and if there isn't for some reason then it seems reasonable to proceed with compilation/execution

Yes additionally, I think this would be a breaking change

dsyme · 2017-12-01T13:41:24Z

src/fsharp/DependencyManager.Integration.fs

+            |> Seq.tryFind (fun a -> a.GetType().Name = attributeName)
+        with | _ -> None
+
+    let getInstanceProperty<'treturn> (theType: Type) indexParameterTypes propertyName =


I believe indexParameterTypes is always empty in this PR, I think it's best to remove it.

dsyme · 2017-12-01T13:42:25Z

src/fsharp/DependencyManager.Integration.fs

+      let executingAssembly = 
+          typeof<IDependencyManagerProvider>.GetTypeInfo().Assembly
+      yield Path.GetDirectoryName(executingAssembly.Location)
+#if FX_NO_APP_DOMAINS


Use #if !FX_NO_APP_DOMAINS

dsyme · 2017-12-01T13:43:51Z

src/fsharp/DependencyManager.Integration.fs

+    interface System.IDisposable with
+        member __.Dispose() = ()
+
+let registeredDependencyManagers = ref None


Does this have to be a static mutable global? Why not hold it in TcConfig or the TcImports (the one for the non-framework assemblies)?

dsyme

Looking good! I made a few comments above. Some more tests needed :)

dsyme · 2017-12-01T18:50:22Z

@KevinRansom @cartermp We should add an RFC for this. Language/tooling suggestion is here: fsharp/fslang-suggestions#542

cartermp · 2017-12-01T19:39:39Z

This is the one that will need reworking: https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1027-fsi-references.md

KevinRansom · 2017-12-01T23:37:50Z

Surely you had an RFC when you did this original work?

smoothdeveloper · 2017-12-05T02:02:11Z

@KevinRansom the RFC is at https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1027-fsi-references.md

realvictorprm · 2018-01-26T10:42:45Z

Ping :-)

When do we plan to add this? End of 2018?(no trolling)

cartermp · 2018-01-26T15:57:42Z

When it's ready.

isaacabraham · 2018-01-26T16:22:41Z

KevinRansom · 2018-01-26T20:04:53Z

@realvictorprm sorry for the lack of progress. This is on me, it's an expected deliverable, but stuff keeps coming up.
Sorry ...

Kevin

realvictorprm · 2018-01-26T20:08:06Z

Many thanks for the response. That's all what I wanted to know so far :-) Am 26.01.2018 9:04 nachm. schrieb "Kevin Ransom (msft)" < [email protected]>:

…

@realvictorprm <https://github.com/realvictorprm> sorry for the lack of progress. This is on me, it's an expected deliverable, but stuff keeps coming up. Sorry ... Kevin — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4042 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYPM8HZpVGkYFo9m2gPaB8BgD0wbOB6dks5tOi_qgaJpZM4QxuqJ> .

amieres · 2018-02-20T23:54:31Z

Would this also support plain old environment variables?

#r "%pack%/WebSharper/lib/net40/WebSharper.Core.dll"

KevinRansom · 2018-02-20T23:59:10Z

That would depend on the package manager.

#r "pathToDll" doesn't, and won't be amended to enable.

however:
#r "nuget:moniker"
moniker won't be a path to disk,

I'm guessing
#r "paket:moniker"

but if someone wrote a filesystem package manager:
#r "filesystem:sometoken"

would, if someone chose to make a filesystem package manager.

I hope this helps

Kevin

amieres · 2018-02-21T00:04:12Z

Yes, it does help.
Thank you.

matthid

Some questions/clarifications. Maybe some comments are required (or another signature for the interface)

matthid · 2018-08-02T17:01:39Z

src/fsharp/CompileOps.fs

+                        | _ ->
+                            match DependencyManagerIntegration.tryFindDependencyManagerByKey m packageManagerKey with
+                            | None ->
+                                errorR(DependencyManagerIntegration.createPackageManagerUnknownError packageManagerKey m)


Yes additionally, I think this would be a breaking change

matthid · 2018-08-02T17:16:26Z

src/fsharp/DependencyManager.Integration.fsi

+    abstract Name : string
+    abstract ToolName: string
+    abstract Key: string
+    abstract ResolveDependencies : string * string * string * string * string seq -> string option * string list


Do I understand correctly that this returns F# code which itself is compiled and run?
Shouldn't we just have a list of implementation and reference assemblies or do we need that for fable somehow?

@matthid Yes, the extension manager, returns a script to FSI that is executed. The interesting thing about this approach is it's very easy to reason about and debug, what is happening. For example we know what happens when you reference an already referenced assembly.

Well I have to say that there is some technical elegance in this.

But is there particular use-case where we need this? Because on the other side it slows down things and might introduce a lot more error scenarios where a user might not realize what happens behind the scenes. (You might argue that performance is probably not a huge deal considering the package-manager might go on the network and I basically agree, on the other side stuff might be cached already and the package manager might return very fast)

@matthid / @KevinRansom it actually returns location of a generated script, it means if any error surfaces, it will point to a temporary file which remains there for debug purpose.

Ok but realistically what can a user do if there is an error in that script? Especially a newcomer will not know where to report the issue. While we can probably pass the context and improve the error message I don't think it is part of this PR (or the spec for that matter).

matthid · 2018-08-02T17:17:21Z

src/fsharp/fsi/fsi.fs

+                    | None -> istate // error already reported
+                    | Some (loadScript, additionalIncludeFolders) -> 
+                        for folder in additionalIncludeFolders do 
+                            tcConfigB.AddIncludePath(m, folder, "")


Why do we actually need include folders when we have the load-Script?

@matthid AFAIR they affect the resolution of subsequently loaded .fsx as well as references.

But what is the difference to the script containing #I?

I suspect the choice to return both the script location and additional include folders was taken in context of ScriptCs integration but don't remember for sure.

I'd need to check where that second value is used in context of FSI.

Other advantages (beside troubleshooting) of having a temporary script loaded is caching (based on a hash of the package manager directive contents), and making it easier to load a graph of things using relative path from the top level temporary script.

Note that I'm not against a temporary script at all. I'm just trying to follow the reasoning, because the spec doesn't really say or list pros/cons or something like that. From my perspective it just makes things more complicated. Also I don't really understand the caching argument because the package manager can still cache a list of paths and so can the compiler?

There is still room to adjust the PackageManager integration signatures, and actually document it better in the code (I see there are no comments about the return value).

One concern though was that the return value has to be plain .net types rather than F# specific or relying on extra assembly, to allow easier interop via reflection from reflection / tooling perspective.

Are you concerned the temporary script is going to be a concern?

KevinRansom and others added 7 commits November 28, 2017 10:55

merge with master

6852007

merge with master

b1fa89a

Merge branch 'master' of https://github.com/Microsoft/visualfsharp in…

3e13554

…to forki-paket

Merge branch 'master' of https://github.com/Microsoft/visualfsharp in…

c662690

…to forki-paket

Fix resources for buildfromsource

dc72b1a

Merge branch 'forki-paket' of https://github.com/KevinRansom/visualfs…

d1d0223

…harp into forki-paket

Cleanup

c45341e

saul reviewed Dec 1, 2017

View reviewed changes

dsyme reviewed Dec 1, 2017

View reviewed changes

dsyme requested changes Dec 1, 2017

View reviewed changes

This was referenced Dec 1, 2017

Extension point for package manager. #2667

Closed

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

Closed

KevinRansom changed the title ~~[WIP] - Build packagemanagement based on Steffan's Package manager design.~~ [WIP] - Build packagemanagement based on Steffen's Package manager design. Dec 2, 2017

Feedback + fix fcs .netstandard build

cc903d7

cartermp mentioned this pull request Dec 5, 2017

[Tooling RFC Update required] Package management endpoint #4062

Closed

matthid reviewed Aug 2, 2018

View reviewed changes

ctaggart mentioned this pull request Feb 11, 2019

Package references in F# scripting fsharp/fslang-suggestions#542

Closed

KevinRansom closed this Jul 29, 2019

KevinRansom deleted the forki-paket branch July 29, 2019 17:05

[WIP] - Build packagemanagement based on Steffen's Package manager design. #4042

[WIP] - Build packagemanagement based on Steffen's Package manager design. #4042

Conversation

KevinRansom commented Dec 1, 2017

Choose a reason for hiding this comment

saul Dec 1, 2017 • edited Loading

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

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

realvictorprm commented Dec 1, 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

dsyme Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

dsyme left a comment

Choose a reason for hiding this comment

dsyme commented Dec 1, 2017

cartermp commented Dec 1, 2017

KevinRansom commented Dec 1, 2017

smoothdeveloper commented Dec 5, 2017

realvictorprm commented Jan 26, 2018

cartermp commented Jan 26, 2018

isaacabraham commented Jan 26, 2018

KevinRansom commented Jan 26, 2018

realvictorprm commented Jan 26, 2018 via email

amieres commented Feb 20, 2018

KevinRansom commented Feb 20, 2018

amieres commented Feb 21, 2018

matthid left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saul Dec 1, 2017 •

edited

Loading

dsyme Dec 1, 2017 •

edited

Loading