Skip to content
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 dependencies #6388

Merged
merged 4 commits into from
Mar 29, 2019
Merged

update FCS dependencies #6388

merged 4 commits into from
Mar 29, 2019

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Mar 28, 2019

This updates the FCS dependencies to

  • .NET 4.5 --> .NET 4.6.1
  • FSharp.Core 4.5.2. --> FSharp.Core 4.6.2

Copy link
Member

@auduchinok auduchinok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think FCS readme, which claims net45 and FSharp.Core 4.0.0.0 are referenced, needs updating too.

@@ -19,7 +19,7 @@ referenced DLLs may change on disk, or referenced files may change.
The ``FSharpChecker`` component from FSharp.Compiler.Service does _not_ actively "listen"
to changes in the file system. However ``FSharpChecker`` _does_ repeatedly ask for
time stamps from the file system which it uses to decide if recomputation is needed.
FCS doesn�t listen for changes directly - for example, it creates no ``FileWatcher`` object (and the
FCS doesn�t listen for changes directly - for example, it creates no ``FileWatcher`` object (and the
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something is wrong with encoding.

@@ -1,3 +1,6 @@
#### 23.0.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be something like 28.0.0 already?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changes need to be RI'd back from FCS repository. I'll ask @baronfel to help with that, thanks

Copy link
Member

@baronfel baronfel Mar 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll take care of that when this merges is integrated, so as to not cause conflicts.

@@ -33,7 +33,7 @@ stays up-to-date when changes are observed.

If you want to more actively listen for changes, then you should add watchers for the
files specified in the ``DependencyFiles`` property of ``FSharpCheckFileResults`` and ``FSharpCheckProjectResults``.
Here�s what you need to do:
Here�s what you need to do:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another few encoding problems here and below.

@cartermp
Copy link
Contributor

What's the rationale for the change here?

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

What's the rationale for the change here?

Nothing definite but "it's probably time to do this". (Well, OK, I noticed again that we couldn't use latest FSharp.Core functions, hence the FSharp.Core update, but it's not critical)

There's no real criteria except that the dependencies must be low enough to allow FCS to be used in sufficiently wide circumstances for the various applications that consume it to be happy.

Happy to take advice from FCS users on whether the dependencies are ok

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

I'll take care of that when this merges is integrated, so as to not cause conflicts.

@baronfel Please also RI any changes from FCS repo back here from time to time, thanks

@cartermp
Copy link
Contributor

Are there any #if net461 or lower versions in the code for FCS? If not, it can just target netstandard. No need to have a .NET Framework target if we're fine with net461 as the base .NET Framework version for non-core and non-mono consumers (which I am fine with, since that's been the base for VS since early 2017 and has likely proliferated across the ecosystem)

@auduchinok
Copy link
Member

Happy to take advice from FCS users on whether the dependencies are ok

From the top of my head: it'll allow using things like EmptyArray in FCS if we try to replace F# lists with arrays in some places.

We'll need to updates some clients like Fantomas but I share the "it's probably time" feeling.

@dsyme
Copy link
Contributor Author

dsyme commented Mar 28, 2019

Are there any #if net461 or lower versions in the code for FCS? If not, it can just target netstandard.

Only .NET 4.7.2+ supported netstandard properly IIRC. Also our NETSTANDARD build may vary slightly

Suggest we go to .NET 4.6.1 for now then deal with moving fully to netstandard2.0 separately

@cartermp
Copy link
Contributor

Only .NET 4.7.2+ supported netstandard properly IIRC

It depends. Up until net472 some type forwards are wrong, but most aren't. So if the APIs we use are type forwarded correctly, there's no issue.

@KevinRansom KevinRansom merged commit f1aee5f into dotnet:master Mar 29, 2019
@nightroman
Copy link
Contributor

nightroman commented Mar 30, 2019

Was the change from net45 to net461 necessary? The latest FSharp.Core requires net45. Windows 8x (still in use) comes with net45x built-in. Windows 10 without updates comes with net46 (updates may not always be assumed).

NB I am not affected too much myself but I develop an OSS project using FCS and users may be affected. This worries me. Ideally, I expect FCS requirements to be aligned with FSharp.Core.

@dsyme
Copy link
Contributor Author

dsyme commented Mar 31, 2019

Was the change from net45 to net461 necessary? The latest FSharp.Core requires net45. Windows 8x (still in use) comes with net45x built-in. Windows 10 without updates comes with net46 (updates may not always be assumed).

We have to move forward at some point. I think design-time tooling relying on .NET 4.5 is very very rare now. What's the tool you are working on?

@dsyme
Copy link
Contributor Author

dsyme commented Mar 31, 2019

Ideally, I expect FCS requirements to be aligned with FSharp.Core.

I don't think this is right in general - there are runtime contexts for F# applications (FSharp.Core) and design-time contexts (F# compiler). FCS tools are nearly always design-time or some other form of immediate coding experience (e.g. REPL etc.)

@nightroman
Copy link
Contributor

@dsyme Thank you.

Do not get me wrong, I am not against this. "Moving forward at some point" is the right thing as such.

I think design-time tooling relying on .NET 4.5 is very very rare now. What's the tool you are working on?

Correct, it's very rare for "development" environment. But when development is finished the result script tools are supposed to work in a portable app (Far Manager), on as much OS's as possible. I am working on FSharpFar, it's a plugin for Far Manager.

@nightroman
Copy link
Contributor

nightroman commented Mar 31, 2019

I don't think this is right in general - there are runtime contexts for F# applications (FSharp.Core) and design-time contexts (F# compiler).

F# compiler is not just design time tool. It's the runner of finished script tools as well (runtime context).

@cartermp
Copy link
Contributor

For the purposes of support we only consider it a design-time tool. That isn't to say we intentionally break scripts processing if it depends on a particular .NET Framework version, but we won't hold back a good enough change for that.

@nightroman
Copy link
Contributor

I am not arguing. But please do not treat F# scripts as development toys. They are F#-app-like tools, too.

@dsyme
Copy link
Contributor Author

dsyme commented Apr 1, 2019

I am not arguing. But please do not treat F# scripts as development toys. They are F#-app-like tools, too.

Thanks for the perspective, that's helpful.

Anyway, I think .NET 4.6.1 is OK for FCS latest. If .NET 4.5 is required then the application can probably sit on an earlier FCS (hence only support F# 4.5, but that's likely ok)

@nightroman
Copy link
Contributor

hence only support F# 4.5, but that's likely ok

Just for information, FWIW, I used FSharp.Core 4.6.x with previous FCS 27.x with no problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants