WIP: Use Arcade's NetCurrent

[vzarytovskii]

This uses arcade's NetCurrent property (or redefines it if we don't use arcade).

This changes majority of product project files. Still no way of automatically getting it to scripts or testing framework. If this passes, I'll take another shot of getting it in testing framework, projects and scripts.

Let's see what'll blow up.

Fixes #16596

[github-actions]

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

[vzarytovskii]

Ok, this passes. I think that's enough to unblock VMR/SB, I can continue with the rest (pushing it to test framework) separately.

This is ready.

[mthalman]

I took the liberty of pushing some updates to get this working in the VMR build.

It will require some additional changes in the installer repo to pass the new switch/property I've added.

[vzarytovskii]

I took the liberty of pushing some updates to get this working in the VMR build.

It will require some additional changes in the installer repo to pass the new switch/property I've added.

Tests are failing with some weird result, I'll look into it.

[vzarytovskii]

I took the liberty of pushing some updates to get this working in the VMR build.
It will require some additional changes in the installer repo to pass the new switch/property I've added.

Tests are failing with some weird result, I'll look into it.

Oh, it seems to either be a fluke, or ordering issue. Either way, I think it's ready

@dotnet/fsharp-team-msft @dotnet/source-build-internal please review

[mthalman]

@vzarytovskii - The change in f0e7c27 causes the VMR build to not work because FSharpNetCoreProductTargetFramework is being set before NetCurrent has been set by Arcade.

[vzarytovskii]

@vzarytovskii - The change in f0e7c27 causes the VMR build to not work because FSharpNetCoreProductTargetFramework is being set before NetCurrent has been set by Arcade.

Hm, it can't be set after, since it's used in the paths. I see what I can do.

[mthalman]

Hm, it can't be set after, since it's used in the paths. I see what I can do.

Can you explain more? I don't understand because nothing in Arcade consumes FSharpNetCoreProductTargetFramework so why does it need to be set before?

[vzarytovskii]

Hm, it can't be set after, since it's used in the paths. I see what I can do.

Can you explain more? I don't understand because nothing in Arcade consumes FSharpNetCoreProductTargetFramework so why does it need to be set before?

It's after that whole block. When we build with plain dotnet, we need to use tfm there.

@mthalman i don't see how moving it down will change things. The block above which I moved it is only used by our contributors. However said block uses TFM, and requires it to be set before.

[vzarytovskii]

@mthalman do you have an example of what's failing?

[ViktorHofer]

Why not move the FsLexPath and FsYaccPath properties into the D.B.targets file? They aren't used anywhere in props.

i don't see how moving it down will change things.

That block that reads from Arcade's NetCurrent property needs the Arcade.Sdk import. So that blocks must be defined after

fsharp/Directory.Build.props

Line 63 in c1773be

<Import Project="FSharpBuild.Directory.Build.props" Condition = " '$(FSharpTestCompilerVersion)' == '' "/>

[vzarytovskii]

Why not move the FsLexPath and FsYaccPath properties into the D.B.targets file? They aren't used anywhere in props.

I probably can, just wanted to keep them together initially.

i don't see how moving it down will change things.

That block that reads from Arcade's NetCurrent property needs the Arcade.Sdk import. So that blocks must be defined after

fsharp/Directory.Build.props

Line 63 in c1773be

<Import Project="FSharpBuild.Directory.Build.props" Condition = " '$(FSharpTestCompilerVersion)' == '' "/>

Ah, I see. Yeah, I can either move it or separate.

[vzarytovskii]

@ViktorHofer @mthalman I have moved things around and I think I've fixed SB/VMR interaction of how we do build props.

[vzarytovskii]

@mthalman If it works for SB/VMR now, is it okay to merge it?

[mthalman]

@mthalman If it works for SB/VMR now, is it okay to merge it?

Yep, good for me. I've verified it runs correctly in the VMR.

[ViktorHofer]

Can this PR be merged in? Asking as the sdk -> installer dependency flow is again blocked because of the fsharp patch: dotnet/installer#18752

[psfinaki]

@ViktorHofer there you go

[ViktorHofer]

Official builds are now failing, possibly because of this change. https://dnceng.visualstudio.com/internal/_build/results?buildId=2386528&view=logs&j=e3072f97-b3f6-597b-f816-087a04d4fd77&t=f21b79f1-207f-5330-979c-4d8382ad4e0f

[vzarytovskii]

Official builds are now failing, possibly because of this change. https://dnceng.visualstudio.com/internal/_build/results?buildId=2386528&view=logs&j=e3072f97-b3f6-597b-f816-087a04d4fd77&t=f21b79f1-207f-5330-979c-4d8382ad4e0f

I don't think it's related. Likely a hiccup.

[ViktorHofer]

@vzarytovskii are you really sure that this didn't regress the build? We still haven't got a new build out with this change. See all the failing runs in https://dnceng.visualstudio.com/internal/_build?definitionId=499&_a=summary

[vzarytovskii]

@vzarytovskii are you really sure that this didn't regress the build? We still haven't got a new build out with this change. See all the failing runs in https://dnceng.visualstudio.com/internal/_build?definitionId=499&_a=summary

I was sure that it's the other flaky test we have. However I'm not entirely sure how did framework change affect the specific e2e tests. I'll look at it

[vzarytovskii]

@vzarytovskii are you really sure that this didn't regress the build? We still haven't got a new build out with this change. See all the failing runs in https://dnceng.visualstudio.com/internal/_build?definitionId=499&_a=summary

I've reverted change for the e2e tests (making them target net8.0 explicitly).

[ViktorHofer]

I've reverted change for the e2e tests (making them target net8.0 explicitly).

Thanks. Curious, will the change flow back into main as well?

[vzarytovskii]

I've reverted change for the e2e tests (making them target net8.0 explicitly).

Thanks. Curious, will the change flow back into main as well?

Automatically - only when 17.10 flows back into main (when 17.11 is branched), but I will cherry-pick it manually.

[vzarytovskii]

It seems, it id't fix it. I will disable e2e tests for now