-
Notifications
You must be signed in to change notification settings - Fork 785
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.
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
Package Management for FSI #5850
Package Management for FSI #5850
Conversation
Since many of you are bound to ask : how to test the coreclr version This is one way that I use:
Then type build: building/whiring and test running will then ensue. If build fails and the failure is not to do with the fsc_tools project, then don't worry, a testable dotnet cli has been created in:
|
Link #2407 |
I scanned the basic shape of the PR and couldn't find major philosophical differences :) It looks great! |
My biggest question is about bootstrapping? How can we make sure F# users
have paket access ootb when they are installing dotnet core. This whole
thing is only interesting if we can get this to work.
Am Do., 1. Nov. 2018, 00:42 hat Don Syme <notifications@github.com>
geschrieben:
… I scanned the basic shape of the PR and couldn't find major philosophical
differences :) It looks great!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5850 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNLAAdVr1QsZwQVW97TLTUYY5gEKkks5uqjVigaJpZM4YE0CS>
.
|
Maybe it can happen, do you have an idea how you might want this to work? It does feel like Paket may need some bootstrap script, user-global paket install or something. |
There are multiple ways to do it
|
I don't think we can put a bootstrapper in the box unless we authored and owned a paket bootstrapper that was subject to all the same requirements that any MS-authored component has. But in general, since this is pluggable, it stands to reason that any other installed tool could also supply a plugin. That could be paket or fake. But @KevinRansom knows more about how it actually plugs in, and this also needs to be documented. But we're a ways off from this being ship-ready anyways. |
Well I already contributed that bootstrapper and it wouldn't be to hard to
do for you from scratch. But I know it's always a hard thing to do for
Microsoft, so that's why we came up with the generic way to load #r
providers in my original PR.
Am Do., 1. Nov. 2018, 18:03 hat Phillip Carter <notifications@github.com>
geschrieben:
… I don't think we can put a bootstrapper in the box unless we authored and
owned a paket bootstrapper that was subject to all the same requirements
that any MS-authored component has.
But in general, since this is pluggable, it stands to reason that any
other installed tool could also supply a plugin. That could be paket or
fake. But @KevinRansom <https://github.com/KevinRansom> knows more about
how it actually plugs in, and this also needs to be documented. But we're a
ways off from this being ship-ready anyways.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5850 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgND1lWgoWXOOWHUktX3Gw7qPMXCLkks5uqylkgaJpZM4YE0CS>
.
|
@forki, we will do whatever we must to make a paket packagemanager usable and work well. Either: Or we will ship an in-box bootstrapping package manager that will go fetch the paket package manager ideally on first use of #r "paket:blah ...." We are committed to make it work and work well. TBH: if paket doesn't work well, Don would have my guts for garters. Kevin |
Usability note from today: You must turn off shadow copying for assemblies in Tools > Options > F# Tools > F# Interactive for this to execute in F# interactive. That makes things difficult, since unless we can resolve the design to not require that be off, we'll either have a "doesn't work by default" experience or a "break some people" experience. |
FYI this currently breaks all editor tooling except for a standalone F# script file. |
Cab you explain the all editor tooling? Only fsi.exe turns on shadow copy and turning that off doesn't break anything as far as I know. Except of course for FSI now locking referenced assemblies. |
File > New Project > any F# project |
No, in the build of master against 15.8.8 with this VSIX installed, no files have language service lightup. What build are you testing against? |
Dogfood (15.9 preview 5). Although I can't imagine why 15.8.8 would be a problem. I'll take a look. |
validatePackageName v "mscorlib" // Even though it's not a real package. | ||
validatePackageName v "FSharp.Core" | ||
validatePackageName v "System.ValueTuple" | ||
validatePackageName v "NETStandard.Library" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may want to consider Microsoft.NETCore.App
and/or whatever gives us access to Span
. I suppose that's a bit tricky, but Iactually just saw some requests about how to use Span with FSI, and the only way to do that here would be to reference the relevant bits.
raise (ArgumentException(sprintf "PackageManager can not reference the System Package '%s'" packageName)) // @@@@@@@@@@@@@@@@@@@@@@@ Globalize me please | ||
|
||
let mutable loggerVerbosity = "" | ||
let references = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom parsing the console log to read the dependencies can be improved.
In fsac/ionide/fable etc, to get the fsc args (so similar to this) we invoke an external msbuild (like here), and to do so we use https://github.com/enricosada/dotnet-proj-info/
That lib/tool invoke an external msbuild, but instead of parsing the log use a different strategy.
To the fsproj is added a target file, with a target who run in design mode, and save the items to file (like a csv).
the target is like https://github.com/enricosada/dotnet-proj-info/blob/master/src/dotnet-proj-info/Inspect.fs#L170-L196 so you can directly use that if you want
The trick is good because there is no parsing of log and work with any fsproj/csproj, and you can read everything you want inside the msbuild target (at any msbuild step, with a single invocation) and in the target write the output file as you wish
Some features: get installed .net frameworks, get fsc args from proj or fsx, get props/items, etc. Will use same design time run, like vs, so will not invoke fsc either.
Works the same for.net core fsproj but also for verbose fsproj (with another trick, replace completly projectcracker), but here is not needed.
If you want, can be used as library (like fsac/fable does), so we can reuse the code too, the library (https://www.nuget.org/packages/Dotnet.ProjInfo/) doesnt have any deps, only fsharp.core
Example use as lib: https://github.com/fable-compiler/Fable/blob/master/src/dotnet/Fable.Compiler/CLI/ProjectCoreCracker.fs#L66-L75
fsac usage is more complicated because does navigate the p2p hierachy too, and gather additional info, but here is not needed
happy to chat about it, ihmo can be a nice way to share and improve a common library and strategy.
Really happy to move it somewhere else (like in fsharp
/dotnet
org) and give you ownership too (plus doing things like strong name/etc if needed).
i'll check how to bootstrap paket. |
src/fsharp/FSharp.DependencyManager/FSharp.DependencyManager.Utilities.fs
Outdated
Show resolved
Hide resolved
@enricosada the mono build shouldn't be under reshaped msbuild. And so this code path is the controlling one which uses msbuild inproc: |
Just confirming that there was something environmental going on my end. This works just fine with 15.8.8. Some usability notes:
|
That shadow copy part is only applying to VS, right? Other editors and fsi
standalone could still work fine?
BTW you should also look at the current implementation of this whole thing
in FAKE. We certainly don't want to break FAKE. /cc @matthid for details
Am Fr., 2. Nov. 2018, 21:46 hat Phillip Carter <notifications@github.com>
geschrieben:
… Just confirming that there was something environmental going on my end.
This works just fine with 15.8.8.
Some usability notes:
-
Needing to specify the version explicitly, though due to a NuGet
limitation, needs to get resolved. nuget: FSharp.Data should always
give the latest stable version, for example.
-
The editor for F# scripts is noticeably slow when using this. Likely a
lot of redundant work going on.
-
Need to find a way not to rely on Shadow Copy being off. This is on by
default for users, and turning it off would regress current workflows for
people. This could end up being a showstopper.
-
Logging done in FSI eventually needs to be cleaned up, but it makes
sense it see it now given that we're very early on.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5850 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNMHs0AvC0L4A3RMuuqbX98WFX1ZRks5urK8SgaJpZM4YE0CS>
.
|
Nope. VS doesn’t use shadowcopy, it’s just FSI that is the PITA. So FSI on the desktop will either have to choose shadowcopy or #r.
However, I think we can run the msbuild stuff in a separate non-shadowcopying app domain, or out of process. Still plenty of options ahead of us. This would certainly not impact the paket provider since it won’t run msbuild in-process with FSI, which is the issue.
It should be remembered that longer term the desktop framework is being de-emphasised so this is kind of moot.
Anyway there
From: Steffen Forkmann <notifications@github.com>
Sent: Friday, November 2, 2018 2:10 PM
To: Microsoft/visualfsharp <visualfsharp@noreply.github.com>
Cc: Kevin Ransom <Kevin.Ransom@microsoft.com>; Mention <mention@noreply.github.com>
Subject: Re: [Microsoft/visualfsharp] WIP --- Package Management for FSI (#5850)
That shadow copy part is only applying to VS, right? Other editors and fsi
standalone could still work fine?
BTW you should also look at the current implementation of this whole thing
in FAKE. We certainly don't want to break FAKE. /cc @matthid for details
Am Fr., 2. Nov. 2018, 21:46 hat Phillip Carter <notifications@github.com<mailto:notifications@github.com>>
geschrieben:
Just confirming that there was something environmental going on my end.
This works just fine with 15.8.8.
Some usability notes:
-
Needing to specify the version explicitly, though due to a NuGet
limitation, needs to get resolved. nuget: FSharp.Data should always
give the latest stable version, for example.
-
The editor for F# scripts is noticeably slow when using this. Likely a
lot of redundant work going on.
-
Need to find a way not to rely on Shadow Copy being off. This is on by
default for users, and turning it off would regress current workflows for
people. This could end up being a showstopper.
-
Logging done in FSI eventually needs to be cleaned up, but it makes
sense it see it now given that we're very early on.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5850 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNMHs0AvC0L4A3RMuuqbX98WFX1ZRks5urK8SgaJpZM4YE0CS>
.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fvisualfsharp%2Fpull%2F5850%23issuecomment-435507851&data=02%7C01%7CKevin.Ransom%40microsoft.com%7C88b046a3aa2348b9b9b408d64107951f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636767898169211059&sdata=c523JVR1ptisyenbSLVCbqY72pMGpn05dBZ2UybkEx4%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAE76FiOKTbZE-9nr6fCwTGj3g2IS4wAyks5urLS2gaJpZM4YE0CS&data=02%7C01%7CKevin.Ransom%40microsoft.com%7C88b046a3aa2348b9b9b408d64107951f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636767898169221067&sdata=iF8nrrbdzog9cUx1fosfmKygDo19SqFFDw0B0zZVMt0%3D&reserved=0>.
|
Technically, this PR is a breaking change. For this to be compatible with fake all we need would be a public api in fcs. Ideally, no extra dll is required in that scenario... |
@matthid, what's broken exactly? |
f4afd65
to
0fcfe15
Compare
I am trying to us this already today. (for my own scripting editor)
|
See my tweet here with code example. Syntax should be https://twitter.com/realvictorprm/status/1173639408693723136?s=19 |
@realvictorprm thanks! Your code example works. How can I use the I used the one from
I still get this error. |
@Goswin, fcs need to deploy: So use the assemblies built into FSharp.Compiler.Private.Scripting I will up0date the fcs build to do the necessary at some point. |
@Goswin, on coreclr, you should also use the latest version of FSharp.Data. Earlier versions don't work correctly due to a bug in the typeprovider sdk. |
@KevinRansom Thank you for you help!
Shall i refrence those instead of the |
yield! resolveDependencyManagerSources filename | ||
#if DEBUG | ||
for (_,subFile) in sources do | ||
printfn "visiting %s - has subsource of %s " filename subFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other conditional enclosed printfn below fails the "load-test" test when run locally (in debug mode by default), could it be removed from master?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the answer to this is to adjust testing all-up, since these kinds of diagnostics are very important for environments like Jupyter (where this is enabled) to see what went wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a #if JUPYTER_NOTEBOOK_DEBUG
is a good approach then? Not a big issue but wanted to report it may be an overlooked remnant.
The diagnostics could also be incorporated in the dependency manager itself (assuming jupyter will have its own) rather than CompileOps.fs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@KevinRansom @dsyme was pointing at the printf here ^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm giving this a shot reviving and adjusting the paket implementation (https://github.com/smoothdeveloper/Paket/tree/fsi-paket-dep-manager), this is working 🙂.
@brettfo / @KevinRansom is there a way to pull the version of master branch compiled fsi.exe out of continuous integration with a stable URI so I can work on integrating the tests in the CI pipeline in paket repository with all the tests we initially had in the experimental branch?
None) | ||
|> Seq.filter (fun a -> ReflectionHelper.assemblyHasAttribute a dependencyManagerAttributeName) | ||
|
||
let registeredDependencyManagers = ref None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reference is never written to, the assemblies and types seems to currently be scanned on each batch of dependency manager lines evaluated.
yield! resolveDependencyManagerSources filename | ||
#if DEBUG | ||
for (_,subFile) in sources do | ||
printfn "visiting %s - has subsource of %s " filename subFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a #if JUPYTER_NOTEBOOK_DEBUG
is a good approach then? Not a big issue but wanted to report it may be an overlooked remnant.
The diagnostics could also be incorporated in the dependency manager itself (assuming jupyter will have its own) rather than CompileOps.fs.
let keyProperty = keyProperty.GetValue >> string | ||
|
||
static member InstanceMaker (theType: System.Type, outputDir: string option) = | ||
match ReflectionHelper.getAttributeNamed theType dependencyManagerAttributeName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the original code, those were nested checks; it looks much better that way, but I believe the first check (attribute on the type) should be handled separately, there are potentially lots of types that will be coming in the assemblies, better to reject them on not having the attribute, then do the remaining checks.
namespace lib" | ||
|
||
let generateProjectBody = @" | ||
<Project Sdk='Microsoft.NET.Sdk'> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like msbuild black belt stuff 😄
with _ -> Seq.empty) | ||
|> Seq.choose (fun path -> | ||
try | ||
Some(AbstractIL.Internal.Library.Shim.FileSystem.AssemblyLoadFrom path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assemblies seem to be locked, would it be possible/better to shadow copy those?
type internal IDependencyManagerProvider = | ||
abstract Name : string | ||
abstract Key: string | ||
abstract ResolveDependencies : scriptDir: string * mainScriptName: string * scriptName: string * packageManagerTextLines: string seq * tfm: string -> bool * string list * string list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider replacing the string list
with arrays or enumerables. this makes it possible to implement the provider without dependency on FSharp.Core.
This was a concern when I made the initial implementation of the provider interface so the same provider could be loaded from scriptcs (@glennblock), ideally the same interface could just work for C# interactive as well (given they implement similar hooks at some point).
The signature should be documented, it is not obvious that return type is:
- success: bool
- load scripts: string []
- include dirs: string []
yield AppDomain.CurrentDomain.BaseDirectory | ||
]) | ||
|
||
let enumerateDependencyManagerAssemblies compilerTools m = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this going to conform to the RFC https://github.com/fsharp/fslang-design/blob/master/tooling/FST-1027-fsi-references.md#handler-resolution or does the RFC need to be adjusted?
@smoothdeveloper, Excellent, now I can see it, thanks |
This is heavily based on the work @forki and @dsyme and others did for this PR: #2483
Thanks guys, that is a genius mechanism.
Notes:
It's a WIP --- there are undoubtedly a ton of bugs, we need to work through the scenarios. I expect there are places where Don and I will have philosophical differences, we usually do :-)
It works on the desktop with fsi.exe and on the coreclr, and in VS. We rely on the dotnet sdk for resolution, essentially we generate a .fsproj and restore it for resolution.
We surface dotnet/msbuild diagnostic messages, using a verbosity filter.
The PM integration point calls the package resolution mechanism with a list of all of the resolution strings specified in the session, which then computes the assemblies to reference.
If resolution fails the last group of references added are removed from the list of resolution strings, after all it made the graph non resolvable. The developer can retype a corrected command.
It is pluggable, the approach and interfaces are very close to the original PR, so I expect Steffen, can rework his paket resolver pretty quick.
ToDo: Currently it doesn't allow a pluggable resolver to be referenced using #r"nuget:" that is my next thing to get working.
This adds a --compilertools: command line option for specifying the path to the root of a nuget package containing a compiler tool, such as a package manager. The package manager is then resolved similarly to type providers, with fsc and fsi computing a run time version and then compatible framework ids to probe for a dll to load.
ToDo: compile single script file doesn't seem to work, since I expected it to … I am slightly confused.
ToDo unify TP and DependencyManager framework detection … probably publicly expose it from FCS.
The Fsharp.Data package works on coreclr and desktop, so this script does what you might expect:
There are some basic tests but many more to do.
How to see it work:
What I do:
With the branch on a machine, build the F# VisualFSharp tools by:
build vs
Run vs by:
devenv /RootSuffix RoslynDev
Create a new script file and start typing:
For example:
Note that openweathermap which is very cool now requires an APIKey, the number of access is quiet limited, so you may get an error back from the message, so, best to grab an api key of your own, then other folk looking at it won't use up your experiment time.
Note the intellisense and syntax colouring:
Select everything and send to interactive: