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

Using msbuild to package FSharpPlus #49

Merged
merged 5 commits into from
Nov 11, 2017

Conversation

wallymathieu
Copy link
Member

@wallymathieu wallymathieu commented Nov 10, 2017

Note also build status on appveyor:
Build status
Need to verify that the package is generated as it should.

@wallymathieu wallymathieu mentioned this pull request Nov 10, 2017
4 tasks
@gusty
Copy link
Member

gusty commented Nov 10, 2017

I see all green. Is it Ok to merge on your side?

@wallymathieu
Copy link
Member Author

Well, we need to set it so that it includes the version in the build, right now it builds 1.0.0, so that needs to be fixed as well.

@gusty
Copy link
Member

gusty commented Nov 10, 2017

I see the error on the build. That’s because FSharpPlus is used to generate the docs.

@wallymathieu
Copy link
Member Author

But I don't get how that change cause that build failure ... since it changes the package prefix, does it also clean the folders perhaps?

@wallymathieu
Copy link
Member Author

Aa, yes, it was due to fiddling with the build script

@wallymathieu
Copy link
Member Author

The thing that looks wrong now is that for netstandard2.0 we probably want the FSharpPlus.BaseLib in that folder.

@@ -21,15 +21,16 @@
</PackageTags>
<ProjectGuid>1368368e-d2f4-4fef-bb2f-492e05156e0f</ProjectGuid>
<GenerateDocumentationFile>true</GenerateDocumentationFile>
<OtherFlags>--staticlink:FSharpPlus.BaseLib --warnon:1182</OtherFlags>
<OtherFlags Condition=" '$(TargetFramework)' == 'netstandard2.0'">--warnon:1182</OtherFlags>
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't compile netstandard package with static linking with FSharpPlus.BaseLib

@wallymathieu
Copy link
Member Author

<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
 <metadata>
   <id>FSharpPlus</id>
   <version>1.0.0-CI00089</version>
   <title>FSharpPlus</title>
   <authors>Gusty</authors>
   <owners>Gusty</owners>
   <requireLicenseAcceptance>false</requireLicenseAcceptance>
   <licenseUrl>http://opensource.org/licenses/Apache-2.0</licenseUrl>
   <projectUrl>https://github.com/gusty/FSharpPlus</projectUrl>
   <description>F#+ is an overload library for F#.</description>
   <tags>f# FSharp Applicative Monad MonadTransformer Arrow Overloading</tags>
   <dependencies>
     <group targetFramework=".NETFramework4.5">
       <dependency id="FSharpPlus.BaseLib" version="1.0.0" exclude="Build,Analyzers" />
       <dependency id="FSharp.Core" version="4.2.3" exclude="Build,Analyzers" />
     </group>
     <group targetFramework=".NETStandard2.0">
       <dependency id="FSharpPlus.BaseLib" version="1.0.0" exclude="Build,Analyzers" />
       <dependency id="FSharp.Core" version="4.2.3" exclude="Build,Analyzers" />
     </group>
   </dependencies>
   <contentFiles>
     <files include="any/net45/App.config" buildAction="Content" />
     <files include="any/netstandard2.0/App.config" buildAction="Content" />
   </contentFiles>
 </metadata>
</package>

is how the nuspec found on appveyor looks like.

@gusty
Copy link
Member

gusty commented Nov 10, 2017

Oh I see, you mean that we can't embed it for netcore ?

@gusty
Copy link
Member

gusty commented Nov 10, 2017

Well if there's nothing we can do about it we should merge it as it is. What do you think?

@gusty
Copy link
Member

gusty commented Nov 11, 2017

I tried to build in my computer. It works but I get some funny warnings:

C:\Repos\FSharpPlus\.paket\Paket.Restore.targets(179,5): warning : Description: The package version '1.0.0-1.0.0-CI00089' uses SemVer 2.0.0 or components of SemVer 1.0.0 .....

@wallymathieu
Copy link
Member Author

How are you building? the latest commits I've done is in order to tell msbuild about versionprefix and suffix, so at least the versions are as it should

@gusty
Copy link
Member

gusty commented Nov 11, 2017

Well, I just clean up everything, clone the repo and run build.
Do you get something different in yours?
If you want I can merge so we check with myget if the versions are ok.

@wallymathieu
Copy link
Member Author

I'm currently away from all windows computers, so I can't try it locally. Though you can check the build result from https://ci.appveyor.com/project/wallymathieu/fsharpplus-8q142/branch/pack_for_net45_on_nix and download the artifacts

@gusty
Copy link
Member

gusty commented Nov 11, 2017

I'm currently away from all windows computers
Lucky you !!!

It looks good, no warnings. Honestly I don’t care about those warnings. If it’s ok for you we can merge.

@wallymathieu
Copy link
Member Author

As long as it's ok for you then merge 😉 I will see if I can figure out how msbuild pack works. Perhaps there is some flag to control the reference so that it is included as a private assembly

@wallymathieu
Copy link
Member Author

When running it locally on an old mac I get:

Done Building Project "/Users/mathieu/src/FSharpPlus/src/FSharpPlus/FSharpPlus.fsproj" (_GetFrameworkAssemblyReferences target(s)).
PaketOverrideNuspec:
  /Library/Frameworks/Mono.framework/Commands/mono --runtime=v4.0.30319 "/Users/mathieu/src/FSharpPlus/.paket/paket.exe" fix-nuspecs files "obj/Release/FSharpPlus.1.0.0-1.0.0-CI00089.nuspec;obj/Release/FSharpPlus.1.0.0-CI00089.nuspec;obj/Release/FSharpPlus.1.0.0.nuspec" project-file "/Users/mathieu/src/FSharpPlus/src/FSharpPlus/FSharpPlus.fsproj" 
  Paket version 5.122.0
  Performance:
   - Runtime: 2 seconds
/Users/mathieu/src/FSharpPlus/.paket/Paket.Restore.targets(179,5): error : Directory "/Users/mathieu/src/FSharpPlus/src/FSharpPlus/obj/Release/FSharpPlus.1.0.0-1.0.0-CI00089.nuspec;/Users/mathieu/src/FSharpPlus/src/FSharpPlus/obj/Release/FSharpPlus.1.0.0-CI00089.nuspec;/Users/mathieu/src/FSharpPlus/src/FSharpPlus/obj/Release" not found. [/Users/mathieu/src/FSharpPlus/src/FSharpPlus/FSharpPlus.fsproj]
Done Building Project "/Users/mathieu/src/FSharpPlus/src/FSharpPlus/FSharpPlus.fsproj" (pack target(s)) -- FAILED.

@wallymathieu
Copy link
Member Author

so perhaps it's something that needs to be fixed in relation to paket

@gusty
Copy link
Member

gusty commented Nov 11, 2017

Most likely. But I will merge it in anycase because it builds and it (hopefully) creates the nuget in myget which allows me to release.
I think you did a great job. Of course if you want to improve it, welcome.

@gusty gusty changed the title [WIP] Using msbuild to package FSharpPlus Using msbuild to package FSharpPlus Nov 11, 2017
@gusty gusty merged commit 4fe1d11 into fsprojects:master Nov 11, 2017
@wallymathieu wallymathieu deleted the pack_for_net45_on_nix branch November 11, 2017 13:08
@wallymathieu
Copy link
Member Author

Thanks! One way of solving it would be to publish a version of FSharpPlus.BaseLib on nuget...

@gusty
Copy link
Member

gusty commented Nov 11, 2017

Maybe is the only way to solve it.
Another way would be to get rid of it, but I don't know if we'll get the same performance for serializations because I think the BaseLib uses unsafe operations.

@gusty
Copy link
Member

gusty commented Nov 11, 2017

Myget still builds version CI00022 :

Verifying package source... Package source verification succeeded.
Initializing build environment... Build environment variables:   BuildRunner=MyGet   
MsBuildExe=C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Community\MSBuild\15.0\Bin\MSBuild.exe   GallioEcho=C:\Program 
Files\Gallio\bin\Gallio.Echo.exe   XUnit192Path=C:\Wonka\testrunners\xunit.runners.1.9.1\tools   
XUnit20Path=C:\Wonka\testrunners\xunit.runner.console.2.0.0\tools   
VsTestConsole=C:\Wonka\testrunners\VSTest.Console.12.0.30723.0\lib\vstest.console.exe   
MsTestExe=C:\Program Files (x86)\Microsoft Visual 
Studio\2017\Community\MSBuild\15.0\Bin\MSTest.exe   SourcesPath=D:\temp\7b1633a   
NuGet=C:\Wonka\nuget\NuGet.CommandLine\tools\NuGet.exe   
NpmExePath=C:\users\wonka\AppData\Roaming\npm\npm.cmd   Configuration=Release   
Targets=Rebuild   Platform=   VersionFormat=1.0.0-CI{0}   BuildCounter=22   PrereleaseTag=CI00022   
PackageVersion=1.0.0-CI00022   EnableNuGetPackageRestore=true
Initializing git-based build environment... Additional build environment variables:   
GitPath=C:\Program Files\Git\bin\git.exe   
GitVersion=C:\ProgramData\Chocolatey\lib\GitVersion.Portable\tools\gitversion.exe

But I think is OK, because I'm not releasing and myget keeps incrementing the CI build, the only think I wonder is why he came back from 89 to 22.
When I release by following the procedure coming from the F# scaffolding project it should be fine.
I will try to release it and see if it works.

@wallymathieu
Copy link
Member Author

wallymathieu commented Nov 12, 2017

When I look at the FSharpPlus.BaseLib it looks as if it's only one class: BitConverter. Perhaps break it out to a separate project BitConverter and publish that on nuget?

@gusty
Copy link
Member

gusty commented Nov 12, 2017

Yes, that’s probably the way to go. I will think what’s the best way to split it.

@gusty
Copy link
Member

gusty commented Nov 14, 2017

Another possibility is to attempt to write it in F# and see if we can get the same performance.
BTW, is this something that it's currently preventing you from using this library?

@wallymathieu
Copy link
Member Author

No, I just wanted to make sure that the nuget package is done done efter the change to netstandard2.0.

@wallymathieu
Copy link
Member Author

What's the reason for BitConverter? Is it that the base library implementations are slow?

@gusty
Copy link
Member

gusty commented Nov 14, 2017

No, I think it's because of the Endianess. I believe those implementations define the Endianess based on the architecture it runs, so they are no referential transparent. As many other functions in the .NET framework they are context dependent.
One of the most important goals of this library is to enforce referential transparency.

@wallymathieu
Copy link
Member Author

Aaa, so even more reason to put it in a separate nuget package. So in other words, if you try to build a serialiser on top of the standard .net class then it will give you headaches when dealing with different machines.

@gusty
Copy link
Member

gusty commented Nov 14, 2017

I think so. I had a project that needed to use Bid Endian regardless of the architecture in order to talk to a third party component.

@gusty
Copy link
Member

gusty commented Nov 30, 2017

@wallymathieu maybe there's a way to code those unsafe blocks in F# , see for example this https://gist.github.com/vietnt/84c7532d70e83dee2545df7507158856 I will give it a try.

@gusty
Copy link
Member

gusty commented Nov 30, 2017

Look at this dotnet/fsharp#3924

So, it seems to be a known issue, to be solved soon.

AFAIK I am the only one using the classes in BaseLib in a performance critical project but at the same time I know I won't be updated anytime soon.

So I propose, in order to avoid duplicate efforts, to go back to the code as it was before adding the optimized version in BaseLib, then release to nuget.

Later when this issue is solved we can use again the code from the BaseLib.

@wallymathieu
Copy link
Member Author

Sounds like a good plan, since then the deliverable will be simple enough for use in .net core et.c.

@gusty
Copy link
Member

gusty commented Nov 30, 2017

Cool, I'll try to have the code adapted ASAP.

@gusty
Copy link
Member

gusty commented Mar 1, 2018

I managed to translate the code in the Baselib to F#, using the same techniques and taking advantage of the new fixed keyword available in F# 4.1.

See 6983947

A quick test showed equivalent performance, so now we can safely get rid of the C# BaseLib project.

@wallymathieu
Copy link
Member Author

Sweet! 👍

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.

2 participants