WIP Fsc module with fix for #689 #1919
Conversation
src/app/Fake.DotNet.Fsc/Fsc.fs
Outdated
| /// References [] | ||
| /// Debug false | ||
| /// ] | ||
| let Compile (fscParams : FscParam list) (inputFiles : string list) : unit = |
There was a problem hiding this comment.
Compile and compile maybe we should decide for one
or maybe have compile and compileWithResult?
There was a problem hiding this comment.
Compile and compile were both functions from the fake4's FscHelper. Initially I was thinking I needed to keep them the same so it wouldn't introduce a breaking change but I just realized this module doesn't exist in fake5 yet so it doesn't matter haha
There was a problem hiding this comment.
Yes we have indeed the opportunity to improve the API and should use it :)
src/app/Fake.DotNet.Fsc/Fsc.fs
Outdated
|
|
||
| /// An externally referenced compiler | ||
| let private externalCompile (fscTool: string) (optsArr: string []) = | ||
| let args = String.Join(" ", optsArr) |
There was a problem hiding this comment.
I think we should use (Arguments.OfArgs optsArr).ToWindowsCommandLine instead of String.Join the former will handle arguments with spaces.
There was a problem hiding this comment.
Is this the case even if using mono in linux? Or should I use linux args style in that case?
There was a problem hiding this comment.
yes, because for cross-platform & compat reasons "netcore" will behave the same.
Mono would behave slightly differently, but fake is not running on mono anymore
There was a problem hiding this comment.
Hey I'm getting a weird name conflict error when trying to use Arguments. In vscode, everything is fine, but when running build.fsx, and in visual studio, the compiler thinks I'm trying to use the discriminated union case DocoptResult.Arguments instead of the one defined in Fake.Core.Process.
The error im getting specifically is
error FS0072: Lookup on object of indeterminate type based on information prior to this program point. A type annotation may be needed prior to this program point to constrain the type of the object. This may
allow the lookup to be resolved.
Both of these Argument types exist in Fake.Core so I'm not really sure how I can inform the compiler of which one it needs to use. Any ideas on how to fix this?
There was a problem hiding this comment.
Nevermind, figured it out. The file with Arguments defined in Fake.Core.Process was not included in the fakelib.fsproj config
There was a problem hiding this comment.
I just fixed the same in release/rc_12 branch
There was a problem hiding this comment.
Changed the PR to be based on release/rc_12, please try to merge latest, sorry for the inconvenience.
| @@ -0,0 +1,4 @@ | |||
| group netcore | |||
|
|
|||
| FSharp.Core | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| <ProjectReference Include="..\Fake.Core.String\Fake.Core.String.fsproj" /> | ||
| <ProjectReference Include="..\Fake.DotNet.MSBuild\Fake.DotNet.MSBuild.fsproj" /> | ||
| </ItemGroup> | ||
| <ItemGroup Condition="'$(TargetFramework)' == 'net46'"> |
There was a problem hiding this comment.
Please remove this item group for FSharp.Compiler.Service (see comment in paket.references)
|
@matthid - If this seems ok to you then I think this pull request is good to go |
src/app/Fake.DotNet.Fsc/Fsc.fs
Outdated
| /// References [] | ||
| /// Debug false | ||
| /// ] | ||
| let CompileWithResult (fscParams : FscParam list) (inputFiles : string list) : int = |
There was a problem hiding this comment.
Can you please use lower-cased first letters for public APIs (these are our current api guidelines), see also Compile, CompileExternalWithResult, CompileExternal
There was a problem hiding this comment.
Oh gotcha, ok, sure thing!
help/templates/template.cshtml
Outdated
| @@ -121,6 +121,7 @@ | |||
| <ul> | |||
| <li><a href="/dotnet-cli.html">Cli</a></li> | |||
| <li><a href="/dotnet-assemblyinfo.html">AssemblyInfoFile</a></li> | |||
| <li><a href="/apidocs/v5/fake-dotnet-fsc.html">MSBuild</a></li> | |||
There was a problem hiding this comment.
You might want to change the text here to Fsc :)
There was a problem hiding this comment.
Doh! My bad haha, I had a feeling I was forgetting something. Thanks
|
Oh yea, one last issue I remembered: When I added FCS to paket references and tried to restore, I was getting an error saying something about System.Runtime is only compatible with .net 4.6.2. So I changed the target from net46 to net462. Also I don't believe it was compatible with netstandard1.6 either. It seems a little out of alignment with the rest of the modules so not sure if there's something wrong with that or not |
|
Thanks a lot for taking care of this! |
|
@vanceism7 41f9142 This is why you had problems with FSharp.Compiler.Service. Basically we had to update the dependencies in order to be able to compile for net462 |
|
@matthid Oh gotcha, I wonder if it will work with net46 though? When I used fcs through nuget, it was working for net46 if I remember correctly. Btw, is it safe for me to delete this branch now? |
|
@vanceism7 Because net46 was already in place.
Yes |
This is fschelper ported to a new fake5 module. Not sure if this is the correct way to call fsc.exe. Also, when calling fsc.exe through a Process call, you lose distinction between errors and warnings that you normally get back in the form of
FSharpErrorInfowhen you call FSharpChecker's compile method. I thought of using regex to scan the error messages for the keywords "warning" or "error", but I figured i'd ask about it first to see if there's a better way to handle this.Also, I'm still unsure if I've referenced FSharp Compiler Service the correct way, although I suspect this setup should work.
Lastly, did we need to apply this fix to the fake4 module as well?