-
Notifications
You must be signed in to change notification settings - Fork 157
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
update FCS, allow fsdocs to roll forward to net5.0 #621
Conversation
dsyme
commented
Dec 1, 2020
•
edited
Loading
edited
- Update FCS to 38.0.0
- Add MSBuild dependencies requried for 38.0.0
- Move consistently to netstandard2.1
- allow fsdocs to roll forward to net5.0
This requires dotnet/fsharp#10575 to pass on .NET 5.0. Until then fsdocs requires an install of .NET Core App 3.1 |
Note that this will require FCS 38.0.1, which we can actually depend on today if we use the public feed instead |
@cartermp The way we're doing FCS is a problem here.
We need to
cc @baronfel |
Hmm, I disagree |
agains 5.0.x maybe, but not a few versions back, that would disallow editors from supporting F# 5 features for example |
Or would it? I dunno. Not worth it though. There's little value in backtargeting an FCS that supports F# X but it reuquires an FSharp.Core that is against X-Y. Reach is not really a thing here. People can just use older FCS versions if they must runa gainst older F# compilers |
Why? The API would be built against 4.7.2 but could run against any later FSharp.Core. If truly reflective FCS-based tooling wants to have their final tool use 5.0.0 that's fine. |
Well, at the moment we can certainly get away with a 5.0.0 API dependency but it should always be something public and on nuget.org. It should not be latest in-repo FSharp.Core beta. Forcing latest dependencies becomes a bigger problem to the extent that
As an example, the current dependency chains are
The emerging set up means that we are forcing that whole chain to to update to latest and greatest, which both forces references to dotnet/fsharp nuget feed betas, and breaks all Ionide F# Analyzers on every update even if FSharp.Compiler.Service were actually binary compatible. That's not the destination we want. The principles of FCS have been as follows:
TBH I think the problem is that dotnet/fsharp has a long cultural habit of forcing latest FSharp.Core on all components it builds - it's like it seems to happen almost automatically in Microsoft repos. The build-from-source in that repo may also part of this (because access to a package server can't be assumed). But it also feels like it's a general engineering/mindset problem - it's so noticeable that as soon as we start building FCS from that repo all the existing principles get upended (and suddenly I'm on the verge of forcing whacky nuget sources and latest beta dependencies on the entire downstream chain for Ionide...) Note many of the same arguments apply to TPSDK - which should have a low FSharp.Core dependency to allow people to continue to make TPs that work with existing F# tooling So in short dotnet/fsharp needs to build FCS and TPSDK without having them be FSharp.Core latest. We also have to get Microsoft.DotNet.DependencyManager published and rely on a stable published version of that, I think that's appreciated too. That should also have a public, stable, low FSharp.Core dependency to allow dependency managers to be built for a range of F# tooling. |
I don't really agree with all of that. We'll have to agree to disagree I think. |
Hmmm :-) Let's talk this over at some point, and I'd like to know specifically what you don't agree with. The issue of dependency management is fundamental to all of FCS, TPSDK, DependencyManager, F# Analyzers. There's simply no point having extension hooks if every update breaks (or requires re-compilation of) all extensions. I also can't resolve this particular issue we're discussing (#621) until the dependency chain for FCS is on nuget.org. I'm not going to ask Ionide and other downstream users to add nuget sources nor beta FSharp.Core references. A beta FCS reference would be ok at a pinch (since FCS is in many ways a beta thing anyway) |
That's actually my point. These are not normal packages. It's been explained to me that things like FSharp.Core is "just" a normal library, when it...literally is required to compile any F# code, and depending on the code, it may not even compile unless you have the right version. Anything involving the analysis of F# source code is not a normal component and cannot be treated as such. Being able to resolve nuget dependencies requires an up-to-date msbuild and nuget component to be able to resolve things correctly, for example. And the "language" for parsing these things out isn't understood by older F# language versions. So why on earth would we want to somehow find a way for older consumers to use this stuff? That's hellish and falls apart the instant anything updates.
This I think is reasonable, though still problematic since we have this issue where some features are partially implemented in FSharp.Core. Analysis of byrefs wouldn't be possible prior to FSharp.Core 4.5.2, for example. I can see a similar issue arising at some point. But I think it's mostly workable. Although it's worth pointing out that in figuring out publishing FCS from dotnet/fsharp, we literally did discuss this and folks were fine with taking on a dependency on nightly releases of FCS via a nuget feed so they could get the latest bits. We've provided that. |
Hmmm maybe I see the disconnect here. The static FSharp.Core dependency of FCS is irrelevant to the actual operation of FCS. In this sense FCS is indeed a normal package with resepect to its static FSharp.Core dependency. It is just a minbound on the surface area of FSharp.Core that FCS needs to run - just as a static dependency of "netstandard2.0" is a minbound on the surface area of .NET that FCS needs to run. It is irrelevant to the language features supported. For example, if FCS statically has a minimum static FSharp.Core 4.7.2 dependency that literally doesn't mean anything about the F# code it can and can't process. Zilch. If I write an F# static analysis tool using FCS 39.0.0 (and any compatible FSharp.Core) then it can do any combination of things
The runtime FSharp.Core in an FCS-based tool (e.g. a test suite or fsdocs) is relevant to the operation of FCS, but only for reflective operations (specifically script execution and the default FSharp.Core reference for processing scripts). |
Yes, in principle I'm "just" ok with this part - FCS is beta and if we really need to occasionally beta-ize the whole dependent ecosystem then I guess that's ok. As I mentioned at the start I know there are a lot of benefits to Microsoft doing the publishing from dotnet/fsharp. I guess I'd be much happier if those betas were on nuget.org. Requiring downstream users (Ionide) to add unusual nuget sources to use latest FSharp.Formatting is not a happy experience. That's just not the kind of component FSharp.Formatting wants to be. However I've always stressed that we need FCS (and Microsoft.DotNet.DependencyManager and TPSDK) to statically depend on well-understood, public minbound-surface-area FSharp.Core. I actually thought we had that already looking at https://github.com/dotnet/fsharp/blob/main/src/fsharp/Directory.Build.props#L10 but that doesn't apply to the CI pakages we're building. I care about the second issue much more than the first. |
To summarize the versions of FSharp.Core relevant to the operation of FSharp.Compiler.Service:
I've added these notes to dotnet/fsharp#10772 |
We managed to get all complaince-related stuff sorted for nuget, so now there are regular updates to fCS flowing again. https://www.nuget.org/packages/FSharp.Compiler.Service/ has everything you need to unblock fsharp.formatting + ionide. I'm be very curious to see if the inclusion of the nuget dependency manager also now means that F# scripts processed via fsharp.formatting could include tooltip info from those nuget references... |
@cartermp This is great news, thank you so much, I'll update now
Looking at the dependencies of https://www.nuget.org/packages/FSharp.Compiler.Service/38.0.2 I don't see Microsoft.DotNet.DependencyManager in that list, I think it's because the files are still included directly in the FCS project? Would we expect that to work? I haven't tried it yet |
It all works as expected, we're using 38.0.2 successfully in FSAC with #r nuget support. |
Cool thanks. I'd imagine plugin dependency resolvers e.g. |
That's right. We'd consider bundling a paket one for convenience I think, but the overall mechanism of handler registration needs something more fleshed out to ensure version consistency between |
@dsyme I meant using |
@cartermp, we could use I think we can stop with the "why do people use paket", just accepting that there are people who do, it will never hurt F#; thinking it does is, I think a bit problematic (like if I said if people use C#, it hurts F# or the reverse). @baronfel, I take your word on bundling |
I don't think I understand. The issue you linked would still affect |
to clarify, I just came for understanding #890 (I think we should allow consumers that are |