Skip to content

Conversation

@OmarTawfik
Copy link
Contributor

No description provided.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wow!!!!! 666 ... this would fail policheck, it needs changing.

Copy link
Contributor

Choose a reason for hiding this comment

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

That tool is hilarious ;-)

@forki
Copy link
Contributor

forki commented Jan 25, 2016

Could you please elaborate a little bit about this change? FSharp.Core + GAC trigger my own internal policheck. Kidding aside: I'd really love to learn more.

@OmarTawfik
Copy link
Contributor Author

@forki This is basically removing F#'s dependency on the GAC, and having its binaries deployed next to the compiler instead of the GAC folder. The flag is to enable/disable copying FSharp.Core.dll to the output folder of the build.

@forki
Copy link
Contributor

forki commented Jan 25, 2016

Is this still compatible with using the FSharp.Core nuget package?
On Jan 25, 2016 21:07, "Omar Tawfik" [email protected] wrote:

@forki https://github.com/forki This is basically removing F#'s
dependency on the GAC, and having its binaries deployed next to the
compiler instead of the GAC folder. The flag is to enable/disable copying
FSharp.Core.dll to the output folder of the build.


Reply to this email directly or view it on GitHub
#894 (comment)
.

@OmarTawfik
Copy link
Contributor Author

@forki This is basically for fsc.exe 4.1. Nothing changed for FSharp.Core binaries except that it will be deployed with the produced binaries (because it is not going to be in the GAC after installing F# on your machine). If You are referencing FSharp.Core from a nuget package, nothing will change there.

@forki
Copy link
Contributor

forki commented Jan 25, 2016

Very good.

What does it mean for "file new project" ?

(sorry for asking so much, but fsharp.core incompatibilities is the biggest
pain in our ecosystem. And I hope you/we can improve that)
On Jan 25, 2016 21:37, "Omar Tawfik" [email protected] wrote:

@forki https://github.com/forki This is basically for fsc.exe 4.1.
Nothing changed for FSharp.Core binaries except that it will be deployed
with the produced binaries (because it is not going to be in the GAC after
installing F# on your machine). If You are referencing FSharp.Core from a
nuget package, nothing will change there.


Reply to this email directly or view it on GitHub
#894 (comment)
.

@OmarTawfik
Copy link
Contributor Author

@forki this means that if you've FSharp.Core.dll as a reference in your project (with CopyLocal=True) it will be copied by msbuild to your output. Else, it won't, and you'll have to add it if you need it elsewhere, because it is no longer in the GAC.
The change is mainly concerned about the fsc.exe scenario, where you're building with the command line. Now the compiler will copy this dll for you, and you can always override it by the flag to disable this functionality (--nocopyfsharpcore).
(I'd certainly welcome questions, or any further comments :) )

OmarTawfik pushed a commit that referenced this pull request Jan 26, 2016
Getting FSharp out of GAC, and add --nocopyfsharpcore flag to fsc.exe
@OmarTawfik OmarTawfik merged commit c301020 into dotnet:master Jan 26, 2016
@OmarTawfik OmarTawfik deleted the copyfsharpcore branch January 26, 2016 02:13
@forki
Copy link
Contributor

forki commented Jan 26, 2016

What I meant is: does the new VS installation still register fsharp.core in
the GAC? If I create a new project, will it reference the GAC version or a
version from the compiler path or a nuget package?
On Jan 26, 2016 3:13 AM, "Omar Tawfik" [email protected] wrote:

Merged #894 #894.


Reply to this email directly or view it on GitHub
#894 (comment).

@enricosada
Copy link
Contributor

About copy FSharp.Core to output dir:

  1. why --nocopyfsharpcore instead of --copyfsharpcore[+|-] ? Like that i cannot explicitly enable it, only disable it.
  2. Now the command line differ by version, with error. i cannot use --nocopyfsharpcore always, i am going to get error FS0243: Unrecognized option: '--nocopyfsharpcore' with a previous version of fsc.
  3. that's done for every version of FSharp.Core? or the vs/msbuild check the version and add the --nocopyfsharpcore based on version of FSharp.Core?
  4. If i create a project and change the fsharp.core version, that's going to copy to ouput dir? or use gacced version?
  5. if i use a specific version of FSharp.Core (nuget/path), that's now copied to output dir? or only if the assembly full name alias (the gac name) it's used?
  6. that's going to happen also for xplat? required from local instead of gac.
  7. The FSharp.Core dll it's the same of compiler right? so coreclr -> coreclr and desktop -> desktop. i cannot compile for another target platform (portable), so we need a check somewhere. Maybe unzipping the FSharp.Core nuget package in a subdir can help, but that's too much probably, i think it's ok an error.
  8. i think this use case is for a simple fsc from command line (it's nice), but the vs templates are going to use the nuget package ( 👍 ) or this simplified version (like old with GAC)?

A note:

if you want some feedback, leave a pr open at least 48h before commit, some of us are from a different timezone, it's really difficult to give feedback like that.
Maybe it's useless, but it's not a big delay (community pr are open much longer and that's not a real problem, more eyes to review)

Also for big changes (like no gac) maybe opening an issue can help the community understand what's happening and why

@forki
Copy link
Contributor

forki commented Jan 26, 2016

Regarding 8)

Personally I'm always using the NuGet package of FSharp.Core and think that's the way to go. But there (at least) two issues:
a) There is currently no way for Microsoft/visualfsharp to produce a proper NuGet package since the mono stuff is not merged from fsharp/fsharp
b) A Microsoft VS template would probably reference via packages.config - which is suboptimal for a big part of the community ;-)

@enricosada
Copy link
Contributor

@forki about 8) i think nuget is the way to go.
a) there is also the problem of coreclr nuget package soon ( see #861 the bonus track ), so having two package of same FSharp.Core depending on platform is bad. And we need anyway the sync of new FSharp.Core versions with mono versions (we can update the ms dll, but we need to wait the sync from visualfsharp -> fsharp and build to create the ~ same package)
b) for sure, but at least the convert to paket work really well 😄

I think if we want to support the simple fsc use case (it's really good to have), we should reference and copy the dll always from the special directory (fsc location is ok), only if NO fsharp.core reference is passed as argument, with a warning.
If the assembly fullname is passed (not the path), we can reference it from fsc location, but it's copied by msbuild.
If the path is passed, reference it, and no copy
so fsc source.fs works, but the other parts doesnt need changes.

@forki
Copy link
Contributor

forki commented Jan 26, 2016

Yes that's what I meant. If this repo creates a nuget package that is not
compatible with mono then we reached maximum confusion. That would be
really really bad. (same for compiler package btw)
On Jan 26, 2016 12:36 PM, "Enrico Sada" [email protected] wrote:

@forki https://github.com/forki about 8) i think nuget is the way to go.
a) there is also the problem of coreclr nuget package soon ( see #861
#861 the bonus track ),
so having two package of same FSharp.Core depending on platform is bad. And
we need anyway the sync of new FSharp.Core versions with mono versions (we
can update the ms dll, but we need to wait the sync from visualfsharp ->
fsharp and build to create the ~ same package)
b) for sure, but at least the convert to paket work really well [image:
😄]

I think if we want to support the simple fsc use case (it's really good to
have), we should reference and copy the dll always from the special
directory (fsc location is ok), only if NO fsharp.core reference is
passed as argument
, with a warning.
If the assembly fullname is passed (not the path), we can reference it
from fsc location, but it's copied by msbuild.
If the path is passed, reference it, and no copy
so fsc source.fs works, but the other parts doesnt need changes.


Reply to this email directly or view it on GitHub
#894 (comment)
.

@enricosada
Copy link
Contributor

the copied core.dll need to be ignored from tests output, i'll add there as a note ( no time to send a pr now )

?? tests/fsharp/core/access/FSharp.Core.dll
?? tests/fsharp/core/array/FSharp.Core.dll
?? tests/fsharp/core/int32/FSharp.Core.dll
?? tests/fsharp/core/libtest/FSharp.Core.dll
?? tests/fsharp/core/quotes/FSharp.Core.dll
?? tests/fsharp/typeProviders/helloWorld/FSharp.Core.dll

@OmarTawfik
Copy link
Contributor Author

@forki Creating a new project from VS didn't change. It would still reference the one in Program Files\Reference Assemblies\Microsoft\FSharp.
@enricosada VS wouldn't pass the flag to the compiler. This is why it is "nocopyfsharpcore" instead of "copyfsharpcore". The compiler would copy it by default. This is mainly to enable the scenario of the user calling the fsc from command line. Now from VS (or msbuild), if you want the referenced FSharp.Core.dll copied to the output folder, you'd have to set CopyLocal/Private to True in the project file.
Starting from F# 4.1, the compiler would look for the referenced FSharp.Core first and copy it, else, it would fall back to the one beside fsc.exe.
It will respect your choice of FSharp.Core (the installed one vs a nuget vs a custom path) if you provide it as a reference to fsc.exe

@dsyme
Copy link
Contributor

dsyme commented Jan 27, 2016

@forki Re nuget packages for FSharp.Core.... This is somewhat off topic for this thread. As @otawfik-ms mentions projects don't actually get FSharp.Core from the GAC (and never have done, at least not deliberately) - they get it from Program Files\Reference Assemblies\Microsoft\FSharp.

But since we're on the topic... If and when Microsoft do push nuget packages for FSharp.Core I expect that they will push a Microsoft.FSharp.Core-net-cli-something-something package following standard Microsoft nuget package naming conventions. This package would then be referenced as a dependency for the existing community FSharp.Core package (which would add the additional FSharp.Core binaries for Xamarin etc.) So existing users of the FSharp.Core package would be happy - and indeed I think everyone will be happy. Do you see any problem with this approach? Thanks

@forki
Copy link
Contributor

forki commented Jan 27, 2016

Making it two packages with a dependency might work if the visualfsharp
generated package doesn't include stuff for platforms that are addressed in
the existing package.
But it will make things pretty hard to understand. We should aim for having
exactly one FSharp.Core package (and exactly one compiler package).
On Jan 27, 2016 1:48 AM, "Don Syme" [email protected] wrote:

@forki https://github.com/forki Re nuget packages for FSharp.Core....
This is somewhat off topic for this thread. As @otawfik-ms
https://github.com/otawfik-ms mentions projects don't actually get
FSharp.Core from the GAC (and never have done, at least not deliberately) -
they get it from Program Files\Reference Assemblies\Microsoft\FSharp.

But since we're on the topic... If and when Microsoft do push nuget
packages for FSharp.Core I expect that they will push a
Microsoft.FSharp.Core-net-cli-something-something package following
standard Microsoft nuget package naming conventions. This package would
then be referenced as a dependency for the existing community FSharp.Core
package (which would add the additional FSharp.Core binaries for Xamarin
etc.) So existing users of the FSharp.Core package would be happy - and
indeed I think everyone will be happy. Do you see any problem with this
approach? Thanks


Reply to this email directly or view it on GitHub
#894 (comment)
.

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