-
Notifications
You must be signed in to change notification settings - Fork 146
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
Migration to .NET Standard 2.0 #360
Conversation
Upgrades to .NET Core 2.0. Makes necessary type anotations for new core 2.0 APIs.
@ChrSteinert SourceLink changed a lot, so you can remove what's in there now. @ctaggart can probably help with the new version of SourceLink. |
Cool, thanks for the update. Will move on without it then (this should make things easier for now ;) ) |
Ok - I guess I am happy with this for now. Would love to get some feedback on this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good! Thank you for your efforts so far. I made a few comments about the build script based on feedback I received from @enricosada. You can see an example of what I mean here.
.travis.yml
Outdated
@@ -1,5 +1,6 @@ | |||
language: csharp | |||
|
|||
mono: latest | |||
dotnet: 2.1.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this require 2.1.4
? If not, shouldn't we keep it as low as possible or use a matrix to test multiple versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'd guess 2.0.3 is the lowest possible.
build.fsx
Outdated
@@ -119,21 +114,16 @@ Target "CopyBinaries" (fun _ -> | |||
// Clean build results | |||
|
|||
Target "Clean" (fun _ -> | |||
!! solutionFile |> MSBuildRelease "" "Clean" |> ignore | |||
// !! solutionFile |> MSBuildRelease "" "Clean" |> ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call dotnet clean
? Is that a thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a thing! :)
build.fsx
Outdated
#endif | ||
|> ignore | ||
!! solutionFile |> MSBuildRelease "" "Restore" |> ignore | ||
!! solutionFile |> MSBuildRelease "" "Build" |> ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use the DotNetCli
module here to run the restore and build, which should be one task with that module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was reverting back to MSBuild for travis to be happy. See this CI run: https://travis-ci.org/fsprojects/FSharpx.Extras/builds/345267943?utm_source=github_status&utm_medium=notification
I would assume this can only be "fixed" with only compiling to netstandard.
Any opinions on dropping net45?
build.fsx
Outdated
) | ||
|
||
#endif | ||
|
||
// -------------------------------------------------------------------------------------- | ||
// Build a NuGet package | ||
|
||
Target "NuGet" (fun _ -> | ||
Paket.Pack(fun p -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the DotNetCli
module to Paket
.
<DocumentationFile>bin\Debug\FSharpx.Extras.xml</DocumentationFile> | ||
</PropertyGroup> | ||
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> | ||
<TargetFramework>net45</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we add netstandard2.0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow - the main part slipped me …
<AssemblyName>FSharpx.Text.StructuredFormat</AssemblyName> | ||
<TargetFrameworkVersion>v4.5</TargetFrameworkVersion> | ||
<TargetFSharpCoreVersion>4.3.0.0</TargetFSharpCoreVersion> | ||
<TargetFramework>net45</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add netstandard2.0
?
<Prefer32Bit>false</Prefer32Bit> | ||
</PropertyGroup> | ||
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "> | ||
<TargetFramework>net45</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test with netcore2.0
?
<OutputType>Library</OutputType> | ||
<RootNamespace>FSharpx.Tests</RootNamespace> | ||
<AssemblyName>FSharpx.Tests</AssemblyName> | ||
<TargetFrameworkVersion>v4.5</TargetFrameworkVersion> | ||
<TargetFSharpCoreVersion>4.3.0.0</TargetFSharpCoreVersion> | ||
<TargetFramework>net45</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test with netcore2.0
?
Adds System.Reflection.Emit.Lightweight for Emit.DynamicMethod on .NET Standard 2.0.
I hope having a fresh lock file is an okish thing to have‽ |
Hmm. Tests succeed, but Pakets reports an error. How unfortunate. |
build.fsx
Outdated
OutputPath = "bin" | ||
Version = release.NugetVersion | ||
ReleaseNotes = toLines release.Notes}) | ||
OutputPath = "bin" }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this will also rebuild without specifying you want to skip it.
Cool - added the |
build.fsx
Outdated
@@ -140,7 +140,8 @@ Target "RunTests" (fun _ -> | |||
Target "NuGet" (fun _ -> | |||
DotNetCli.Pack(fun p -> | |||
{ p with | |||
OutputPath = "bin" }) | |||
OutputPath = "bin" | |||
AdditionalArgs = [ "--no-build" ] }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@forki, @enricosada, or anyone else want to weigh in before we merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some feedback.
for sourcelink you can do like FSAC ( https://github.com/fsharp/FsAutoComplete/blob/master/src/Directory.Build.props ) or directly add these in paket (should work now afaik, target will be imported correctly for privateassets)
About commands, probably is easyer to call all targets against the solution, instead for earch project.
result is pretty much the same, but is faster. You call on this, no hard feeling. my idea is dotnet build FSharpx.Extras.sln
or just dotnet build
from root should do (same for dotnet test
, dotnet pack
, etc).
</PropertyGroup> | ||
<PropertyGroup Condition="$(TargetFrameworkVersion) == 'v4.0'"> | ||
<DefineConstants>NET40</DefineConstants> | ||
<DocumentationFile>bin\$(Configuration)\$(TargetFramework)\$(AssemblyName).xml</DocumentationFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is old style 😄
you can replace with
<GenerateDocumentationFile>true</GenerateDocumentationFile>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, have never seen that. That is awesome. Will put it in!
src/FSharpx.Extras/paket.template
Outdated
@@ -21,5 +21,6 @@ summary | |||
FSharpx.Extras implements general functional constructs on top of the F# core library. Its main target is F# but it aims to be compatible with all .NET languages wherever possible. | |||
description | |||
FSharpx.Extras implements general functional constructs on top of the F# core library. Its main target is F# but it aims to be compatible with all .NET languages wherever possible. | |||
|
|||
files | |||
src/FSharpx.Extras/bin/Release/net45/* ==> lib/net45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you really need that? you can generate with dotnet pack
. paket support that (will use paket refs)
</PropertyGroup> | ||
<PropertyGroup Condition="$(TargetFrameworkVersion) == 'v4.5'"> | ||
<DefineConstants>NET45</DefineConstants> | ||
<DocumentationFile>bin\$(Configuration)\$(TargetFramework)\$(AssemblyName).xml</DocumentationFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like previous, use <GenerateDocumentationFile>true</GenerateDocumentationFile>
@@ -21,5 +21,6 @@ summary | |||
FSharpx.Extras implements general functional constructs on top of the F# core library. Its main target is F# but it aims to be compatible with all .NET languages wherever possible. | |||
description | |||
FSharpx.Extras implements general functional constructs on top of the F# core library. Its main target is F# but it aims to be compatible with all .NET languages wherever possible. | |||
|
|||
files | |||
src/FSharpx.Text.StructuredFormat/bin/Release/net45/* ==> lib/net45 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same dotnet pack
One additional thing I noticed; |
@ChrSteinert probably you can move all the |
OK - so move away from Pakets pack to .NET. Will do that. |
@enricosada How does it look now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Any chance to get this published to NuGet? |
@enricosada or @forki, do either of you know how publishing works now? I haven't done much with this set of projects in quite some time. |
@panesofglass would you like to become a maintainer of this project? (I can add you to NuGet package owners) I guess that @forki does not mind |
This upgrades the project to .NET Core 2.0.
Also, necessary type anotations for new core 2.0 APIs had been made.
Reenable SourceLink