Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make FSharp.Build build netstandard2.0 #10626

Merged
merged 8 commits into from
Dec 8, 2020

Conversation

KevinRansom
Copy link
Member

@KevinRansom KevinRansom commented Dec 6, 2020

There is no good reason for not building this netstandard2.0.

So making it happen.

@KevinRansom KevinRansom requested a review from brettfo December 6, 2020 07:16
@KevinRansom KevinRansom force-pushed the makefsharpbuildnetstandard branch from 5e9e385 to 8fbaa10 Compare December 6, 2020 09:25
@@ -6,9 +6,6 @@
</PropertyGroup>

<ItemGroup>
<Projects Include="src\fsharp\FSharp.Build\FSharp.Build.fsproj">
<AdditionalProperties Condition="'$(OS)' == 'Unix'">TargetFramework=netcoreapp3.1</AdditionalProperties>
</Projects>
<Projects Include="src\fsharp\fsc\fsc.fsproj">
<AdditionalProperties Condition="'$(OS)' == 'Unix'">TargetFramework=netcoreapp3.1</AdditionalProperties>
Copy link
Contributor

Choose a reason for hiding this comment

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

can this now be moved to the upper property group unconditionally?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, this is proto.proj, was this intended?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was

@KevinRansom KevinRansom force-pushed the makefsharpbuildnetstandard branch from 8fbaa10 to 285c2c7 Compare December 7, 2020 06:18
<TargetFrameworks Condition="'$(ProtoTargetFramework)' == ''">net472;netcoreapp3.1</TargetFrameworks>
<TargetFrameworks Condition="'$(OS)' == 'Unix'">netcoreapp3.1</TargetFrameworks>
<TargetFramework Condition="'$(Configuration)' != 'Proto'">netstandard2.0</TargetFramework>
<TargetFrameworks Condition="'$(Configuration)' == 'Proto'">netstandard2.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

These should be collapsed into a single <TargetFramework>netstandard2.0</TargetFramework>.

Copy link
Member Author

Choose a reason for hiding this comment

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

@brettfo , yeah ... no. We publish and publish requires either a single tfm or a specified framework, I couldn't figure out how to specify a framework on the unix build.
A single tfm in the product build fails with missing tfms. I wasted days trying to figure out something that would actually work. But we can get together and you can experience the frustration of it if you would like,

<PackageReference Include="Microsoft.Build.Framework" Version="$(MicrosoftBuildFrameworkVersion)" />
<PackageReference Include="Microsoft.Build.Tasks.Core" Version="$(MicrosoftBuildTasksCoreVersion)" />
<PackageReference Include="Microsoft.Build.Utilities.Core" Version="$(MicrosoftBuildUtilitiesCoreVersion)" />
<PackageReference Include="Microsoft.Build" Version="$(MicrosoftBuildFrameworkVersion)" PrivateAssets="all" />
Copy link
Member

Choose a reason for hiding this comment

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

Keep the package name and variable name in sync.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooo good eyes.

@@ -17,7 +17,7 @@
<AdditionalProperties>TargetFramework=netcoreapp3.1</AdditionalProperties>
</ProjectReference>
<ProjectReference Include="..\FSharp.Build\FSharp.Build.fsproj">
<AdditionalProperties>TargetFramework=netcoreapp3.1</AdditionalProperties>
<AdditionalProperties>TargetFramework=netstandard2.0</AdditionalProperties>
Copy link
Member

Choose a reason for hiding this comment

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

If the TFM of FSharp.Build is only netstandard2.0, this shouldn't be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah you would think .... however ... the proto project is a bit wierder than it looks, like above we can get together and you can experience the wierdness if you like.

@@ -5,7 +5,6 @@
<PropertyGroup>
<OutputType>Library</OutputType>
<TargetFrameworks>net472;netstandard2.0</TargetFrameworks>
<TargetFrameworks Condition="'$(OS)' == 'Unix'">netstandard2.0</TargetFrameworks>
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this cause it to fail on mac/linux?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was a mistake, I need to revert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Although no, it doesn't and I don't really understand why.

Copy link
Member Author

Choose a reason for hiding this comment

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

@brettfo, you can build net472 on linux. You can't run it, but you can for sure build it.

@KevinRansom
Copy link
Member Author

Thanks brett, I appreciate it. I wish our build were simpler.

@KevinRansom KevinRansom merged commit b4cc1c8 into dotnet:main Dec 8, 2020
nosami pushed a commit to xamarin/visualfsharp that referenced this pull request Feb 23, 2021
* Make FSharp.Build build netstandard2.0

* typo

* fixes

* temp

* display environment windows

* feedback

* test fails
@KevinRansom KevinRansom deleted the makefsharpbuildnetstandard branch January 21, 2022 09:06
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.

3 participants