dotnet / fsharp

The F# compiler, F# core library, F# language service, and F# tooling integration for Visual Studio
https://dotnet.microsoft.com/languages/fsharp
MIT License
3.88k stars 783 forks source link

Fix UseLocalCompiler.Directory.Build.props to invoke dotnet.exe #17542

Closed T-Gro closed 1 month ago

T-Gro commented 1 month ago

Before, the setup was yielding: image image

This makes sure that compilation happens by: dotnet.exe $pathToLocalFsc.dll

github-actions[bot] commented 1 month ago

:white_check_mark: No release notes required

T-Gro commented 1 month ago

That doesn't look correct. What are the exact circumstances it's happening under?

Exact circumstances when I was testing in: Copy over the original version of the file to a new folder ;; rename it to Directory.Build.props, set compiler path. And then just plain dotnet new .. ;; it was failing.

This change is needed so that the invocation goes as "dotnet.exe ../fsc.dll"

vzarytovskii commented 1 month ago

That doesn't look correct. What are the exact circumstances it's happening under?

Exact circumstances when I was testing in:

Copy over the original version of the file to a new folder ;; rename it to Directory.Build.props, set compiler path.

And then just plain dotnet new .. ;; it was failing.

This change is needed so that the invocation goes as "dotnet.exe ../fsc.dll"

That shouldn't be happening. It means that build task confuses the fact that it's running under corehost

T-Gro commented 1 month ago

That doesn't look correct. What are the exact circumstances it's happening under?

Exact circumstances when I was testing in: Copy over the original version of the file to a new folder ;; rename it to Directory.Build.props, set compiler path. And then just plain dotnet new .. ;; it was failing. This change is needed so that the invocation goes as "dotnet.exe ../fsc.dll"

That shouldn't be happening. It means that build task confuses the fact that it's running under corehost

I think it did that because some essential settings get skipped when you DisableAutoSetFscCompilerPath, there is block of props. Setting the dotnet path and file are two of them.

vzarytovskii commented 1 month ago

Hm, I'll look at it tomorrow, that's really weird. I have never seen it happening locally

Martin521 commented 1 month ago

Interesting. After adding these two lines in FSharp.Compiler.Service.fsprop, I sometimes get the unrecognized option /checknulls errors and sometimes not. Haven't found out why.

(This is for git clean -xdf & dotnet build FSharp.Compiler.Service)

T-Gro commented 1 month ago

Interesting. After adding these two lines in FSharp.Compiler.Service.fsprop, I sometimes get the unrecognized option /checknulls errors and sometimes not. Haven't found out why.

Without your /artifacts contents changing? (i.e. not doing rebuilds in between, either CLI or from VS?)

Martin521 commented 1 month ago

I do git clean -xdf & dotnet build FSharp.Compiler.Service

I get the error for FSharp.Build and FSharp.Compiler.Service (both TFMs), the other projects succeed.

Martin521 commented 1 month ago

Without your /artifacts contents changing? (i.e. not doing rebuilds in between, either CLI or from VS?)

Possibly Ionide doing a build? I haven't found anything explicit though. But I definitely sometimes have and sometimes have not the unrecognized option /checknulls errors. The added lines in the fsproj don't make a difference, so forget that remark.

vzarytovskii commented 1 month ago

I can't reproduce it in my machine no matter what I do :|

Martin521 commented 1 month ago

Right now it`s working, let me observe if I can find something more specific.

T-Gro commented 1 month ago

@Martin521 : Did it get better with the changes proposed here? If yes, can I merge this change to be the new suggested template for using locally built compiler?

Martin521 commented 1 month ago

@Martin521 : Did it get better with the changes proposed here?

I haven't seen the issue that I mentioned any more, and it was not really related to the file you changed, just similar symptoms. So, I am not sure if I can give any advice here.

I can create a new project in my environment tomorrow and test it with and without your changes, if that is helpful.

Martin521 commented 1 month ago

I have tested this in a new setup (in dev containers) and can confirm both, that the error message shown in the OP appears without the fix, and that the proposed fix works. So, yes, this PR should be merged.

T-Gro commented 1 month ago

Thanks @Martin521 for testing and verifying, this definitely helps