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

[T-Gro]

Before, the setup was yielding:

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

[github-actions]

✅ No release notes required

[Martin521]

Thanks! I was just struggling with this.
The same should be added in FSharp.Compiler.Service.fsproj after line 60, because else the same error still occurs for dotnet build of that project.
(Or in Directory.Build.props?)

[vzarytovskii]

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

[T-Gro]

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]

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]

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]

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

[Martin521]

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]

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]

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]

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]

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

[Martin521]

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

[T-Gro]

@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]

@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]

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]

Thanks @Martin521 for testing and verifying, this definitely helps