Update C# wrapper to support both desktop and UWP projects... #283

pre10der89 · 2016-12-14T19:24:13Z

The C# wrapper project has been changed to a NETStandard (1.1) PCL allowing it to be consumed by any project targeting .NET4.5 or greater including .NETCore and UWP projects... The C# wrapper uses P/Invoke to call into the Native Yoga DLL...

The "Yoga" C++ project has been updated to support ARM builds...

Added the ability to generate nuget packages for the C# wrapper that supports copying the native DLLs to the target output directory.

… target

…o csharp_wrapper

…al tests (for now)

facebook-github-bot · 2016-12-14T19:24:24Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

pre10der89 · 2016-12-14T19:26:53Z

csharp/Build/Facebook.Yoga.NativeInterop.targets

@@ -0,0 +1,131 @@
+<?xml version="1.0" encoding="utf-8" ?>


This target is used by the test programs to ensure that the proper Native DLL is copied to current PlatformTarget output path... Normally would be done by a Nuget package, however, since the C# wrapper, native DLL, and test project are all in the same solution we include this target in the test project's csproj.

pre10der89 · 2016-12-14T19:29:47Z

csharp/Facebook.Yoga.Desktop.sln

@@ -0,0 +1,102 @@
+


Added a solution for desktop, universal, and all.... The desktop version contains the desktop test project, but does not contain the universal test project. The Facebook.Yoga solution contains both the desktop and universal tests...

Are you building the NuGet package from the two separated solutions or just from Facebook.Yoga.sln?

In reply to: 92469842 [](ancestors = 92469842)

The Universal specific solution is only really useful if you want to run the universal tests... You can build the necessary binaries by using any of the three solutions... I'd be open to opinions on whether having three solutions is 1) a good idea or 2) more hassle than its worth.

Since the Facebook BUCK does not support building Visual Studio solutions, I did not know of a strategy for building the nuget package automatically... Input would be greatly appreciated :)

I know nothing about BUCK, but can't it invoke arbitrary command line tools? xbuild/msbuild?

pre10der89 · 2016-12-14T19:30:35Z

csharp/Facebook.Yoga.Universal.sln

@@ -0,0 +1,112 @@
+


Only difference from the Facebook.Yoga.Desktop solution and this one is that the Universal tests are included here and the desktop tests are not.

pre10der89 · 2016-12-14T19:30:58Z

csharp/Facebook.Yoga.sln

@@ -5,42 +5,124 @@ VisualStudioVersion = 14.0.25420.1
 MinimumVisualStudioVersion = 10.0.40219.1


This solution contains all projects including both the universal and desktop tests.

pre10der89 · 2016-12-14T19:31:45Z

csharp/Facebook.Yoga/Facebook.Yoga.csproj

@@ -0,0 +1,135 @@
+<?xml version="1.0" encoding="utf-8"?>


This is a NETStandard 1.1 PCL project that allows us to build one DLL that targets .NET4.5 and above...

pre10der89 · 2016-12-14T19:32:47Z

csharp/Facebook.Yoga/Facebook.Yoga.xproj

@@ -1,21 +0,0 @@
-<?xml version="1.0" encoding="utf-8"?>


Removing this xproj in favor of the NETStandard PCL approach...

pre10der89 · 2016-12-14T19:34:09Z

csharp/Facebook.Yoga/Native.cs

@@ -20,265 +20,265 @@ internal static class Native
        private const string DllName = "yoga";
 #endif

-        [DllImport(DllName)]
+        [DllImport(DllName, ExactSpelling = true, CallingConvention = CallingConvention.Cdecl)]


Adding additional attributes to the DLLImport statement... The calling convention ensures that there are no PInovke stack imbalance exceptions...

What is not clear is whether these additional qualifiers will affect a MONO build?

pre10der89 · 2016-12-14T19:34:54Z

csharp/Facebook.Yoga/Properties/AssemblyInfo.cs

 [assembly: Guid("75bb7605-e54b-4ede-8f5a-ff1f24464236")]

 [assembly: NeutralResourcesLanguage("en")]

 [assembly: AssemblyVersion("3.0.0.0")]
+[assembly: AssemblyFileVersion("3.0.0.0")]


No idea what version to use... 3.0.0.0 is arbitrary.

Does it make sense for this version to match the version on NPM? Or at least it should match the version on NuGet, which will likely start at 1.0.0?

I'm open to any direction on the version to use... Since the name of the project has changed resetting the NuGet version and the corresponding assembly versions to 1.0.0.0 (1.0.0-pre) seems fine to me...

pre10der89 · 2016-12-14T19:35:24Z

csharp/Facebook.Yoga/project.json

-    }
-  }
-}
+    "netstandard1.1": {}


NETStandard 1.1 is the minimum that can be used with DLLImport

pre10der89 · 2016-12-14T19:35:46Z

csharp/Yoga/Yoga.vcxproj

@@ -1,10 +1,18 @@
 <?xml version="1.0" encoding="utf-8"?>
 <Project DefaultTargets="Build" ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <ItemGroup Label="ProjectConfigurations">
+    <ProjectConfiguration Include="Debug|ARM">


Added ARM configuration

pre10der89 · 2016-12-14T19:36:10Z

csharp/Yoga/Yoga.vcxproj

@@ -22,7 +30,8 @@
    <ProjectGuid>{0446C86B-F47B-4C46-B673-C7AE0CFF35D5}</ProjectGuid>
    <Keyword>Win32Proj</Keyword>
    <RootNamespace>Yoga</RootNamespace>
-    <WindowsTargetPlatformVersion>10.0.14393.0</WindowsTargetPlatformVersion>
+    <WindowsTargetPlatformVersion>8.1</WindowsTargetPlatformVersion>
+    <WindowsSDKDesktopARMSupport>true</WindowsSDKDesktopARMSupport>


Secret flag allowing Visual Studio to build ARM

pre10der89 · 2016-12-14T19:36:41Z

csharp/Yoga/Yoga.vcxproj

@@ -71,15 +99,33 @@
  <PropertyGroup Label="UserMacros" />
  <PropertyGroup Condition="'$(Configuration)|$(Platform)'=='Debug|Win32'">
    <LinkIncremental>true</LinkIncremental>
+    <OutDir>bin\$(PlatformTarget)\$(Configuration)\</OutDir>


Output directory to match pattern of the c# wrapper and tests

pre10der89 · 2016-12-14T19:37:22Z

csharp/Yoga/resource.h

@@ -0,0 +1,14 @@
+//{{NO_DEPENDENCIES}}


Added version resource to Yoga.DLL

What are Yoga.rc and resource.h used for?

In reply to: 92471469 [](ancestors = 92471469)

They apply a version to the native DLL

pre10der89 · 2016-12-14T19:38:19Z

csharp/nuget/Facebook.Yoga.Native.nuspec

@@ -0,0 +1,38 @@
+<?xml version="1.0" encoding="utf-8"?>


Nuget spec file that will create a native only package for the Yoga.DLL... This follows the pattern used by sqlite.native

pre10der89 · 2016-12-14T19:39:08Z

csharp/nuget/Facebook.Yoga.Native.nuspec

+    <file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
+
+    <!-- lib -->
+    <file src="_._" target="lib\netstandard1.0"/>


The inclusion of . as the "lib" allows us to reference this nuget package in a .NET project... This is the model used by sqlite.native

This seems like magic, do you have any documentation on why this works?

In reply to: 92471862 [](ancestors = 92471862)

@rozele It sort of is magic and it seems hard to find definitive documentation on this technique... I originally saw it in sqlite.native and many other libraries that referenced sqlite related packages... Libraries like Microsoft.BCL also use this technique (see its win8 folder)....

A search for . in the NuGet gitHub reveals the constant "PackageEmptyFileName" and that constant is used in the logic of NuGet packager/client...

From what I can gather the NuGet client needs to have some lib\targetframeworkmoniker (TfxM) in order for it to make the package reference-able from another .NET project. Since this nuspec represents a package with only the native DLL we don't actually have a managed library to place into the folder. Nuget automatically strips out empty folders so we cannot leave an empty TfxM folder... To avoid NuGet from excluding the empty folder we put an . file to create a non-empty folder. And to avoid copying the useless . file to the the NuGet source ignores it...

There is a link here that sort of discusses it...

pre10der89 · 2016-12-14T19:40:40Z

csharp/nuget/Facebook.Yoga.Native.targets

@@ -0,0 +1,36 @@
+<?xml version="1.0" encoding="utf-8" ?>


target file associated with the native nuget package... This will copy the appropriate DLL to the current Platform Targets output path... In the x86, x64, arm cases the specific dll is copied... If AnyCPU then all are copied and it is up to the consumer to sort out which DLL to use.

pre10der89 · 2016-12-14T19:41:18Z

csharp/nuget/Facebook.Yoga.nuspec

@@ -0,0 +1,46 @@
+<?xml version="1.0" encoding="utf-8"?>


Nuget spec for the C# wrapper and its associated native dlls... This is uses the sqllite style packaging...

pre10der89 · 2016-12-14T19:42:32Z

csharp/tests/Facebook.Yoga.Desktop.Tests/Facebook.Yoga.Desktop.Tests.csproj

@@ -0,0 +1,103 @@
+<?xml version="1.0" encoding="utf-8"?>


Allows the desktop tests to be run on windows.... The generated tests target NUnit 2.6.4 requiring this project to utilize that Nuget version...

pre10der89 · 2016-12-14T19:44:00Z

csharp/tests/Facebook.Yoga.Universal.Tests/Facebook.Yoga.Universal.Tests.csproj

@@ -0,0 +1,146 @@
+<?xml version="1.0" encoding="utf-8"?>


It was not possible to use the generated tests in the universal test project because those tests require NUnit 2.6.4 which does not provide a UWP NuGet package... A subset of tests has been added and converted to MSTest to allow some tests to run.

I'd definitely like to do a follow-up PR to make the unit tests unit NUnit 3.x package, and that nuget package does have UWP support. I appreciate keeping this PR as narrow as possible given the scope.

JoelMarcey · 2016-12-22T15:47:15Z

@pre10der89 What is the company that the CLA would be under?

zpao · 2016-12-22T21:03:27Z

We had a bit of backlog in the corporate CLA processing but we're caught up so you should be ok (and this comment should trigger the bot to come in an comment).

facebook-github-bot · 2016-12-22T21:08:29Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

matthargett · 2016-12-22T22:06:00Z

ETA for merge? We'd like this updated package published so we can integrate it into the next React-Native-Windows release. microsoft/react-native-windows#941

splhack · 2016-12-22T22:21:17Z

@matthargett maybe today (or tomorrow)!

splhack · 2016-12-23T07:09:29Z

@matthargett unfortunately we're having some tool issue now, and holiday season, maybe it'll take a bit longer...

JoelMarcey · 2016-12-23T16:25:13Z

@matthargett I am going to try to resolve the problem for this particular issue sometime between today and the end of the weekend.

splhack · 2016-12-23T17:40:05Z

@pre10der89 do you mind to rebase this PR to the master again? #297 changed Native.cs and project.json.

JoelMarcey · 2016-12-24T16:15:16Z

@pre10der89 Once you rebase this pull request, I have an idea on how we can get this imported internally successfully, and thus landed. Thanks!

rmarinho · 2017-01-02T14:37:14Z

Any progress on this?

JoelMarcey · 2017-01-03T15:04:55Z

Once this pull request is rebased, I should be able to get this synced up manually.

pre10der89 · 2017-01-03T18:40:54Z

@JoelMarcey I've rebased my PR and updated the project with the new files... All tests appear to pass... Please let me know if you see any issues...

emilsjolander · 2017-01-04T12:04:07Z

The failing tests are unrelated. Another PR is currently working on solving those. @JoelMarcey go ahead and import

emilsjolander · 2017-01-04T12:40:07Z

Travis should be fixed now. If you rebase all tests should pass.

rmarinho · 2017-01-04T13:01:25Z

@pre10der89 can you rebase so @JoelMarcey can import

JoelMarcey · 2017-01-04T15:07:31Z

@rmarinho This was rebased yesterday by @pre10der89. Are we waiting for another rebase or can I go on and try to manually import?

splhack · 2017-01-04T17:26:42Z

@JoelMarcey we can go on!

JoelMarcey · 2017-01-04T18:47:48Z

D4381455 internally

JoelMarcey · 2017-01-05T01:03:27Z

Looks like this is good now. Just having @emilsjolander look at the internal diff for a final check and then we can ship.

rmarinho · 2017-02-03T17:23:22Z

Hey @pre10der89 can you email me to try some packages ? me at ruimarinho.net

pre10der89 · 2017-02-03T19:31:30Z

@rmarinho The package has been published to NuGet... Search for Facebook.Yoga...

emilsjolander · 2017-02-03T19:35:43Z

@pre10der89 By whom? Would love to get us in charge of that so that we can make sure to keep it up to date with the latest changes.

rmarinho · 2017-02-03T20:18:59Z

yeah @pre10der89 i saw that.. but that's missing all the other platforms.. what i wanted is help trying the packages im creating via CI for.. i have a VSTS setup that's working building the projects both on OSX and Windows, and the idea is nuget will support Mac, iOS, Android, as well as the windows platforms and UWP ..

matthargett · 2017-02-03T20:33:42Z

Hey @emilsjolander , @rozele created a private repository from which the deploy scripts and secrets are stores (with Yoga as a submodule), and added us (BlueJeans) as a member. I was under the impression he did that after synchronizing with someone on the Facebook side about the deployment plans, but I'm sorry if we moved forward without that loop being closed. @rozele should be back online next week from his paid time-off, and he can add you to the private repository or transfer ownership as need be.

matthargett · 2017-02-03T20:38:14Z

@rmarinho you can definitely add more platforms to the nuget package so it's reference-able in Xamarin Studio/VS2017 Mac, and there's actually interest from @MartinJohns to add Mac (and Linux) support to the ChakraCore nuget package we contributed there. We're happy to support that work by reviewing changes and getting basic test coverage in a CI like we have elsewhere so all platforms stay well-supported. Advanced NuGet has a bit of a learning curve, as @pre10der89 can attest, but can be very powerful for delivering an awesome developer experience. Let us know how we can help :)

emilsjolander · 2017-02-03T20:42:42Z

@matthargett ok good to know, i did not know about this. Can you set up an email thread between us so we can get that sorted out? I'm emilsj @ fb

pre10der89 added 10 commits December 12, 2016 10:34

Updaing project types, adding test programs

3886767

Added better native interop build target, updated projects to use new…

91fd484

… target

Making non AnyCPU build cases more explicit

3f81edf

Adding Desktop and universal specific solutions

e7618cc

Removing test programs, adding simple package script

7f8cdc5

Merge branch 'master' of https://github.com/pre10der89/css-layout int…

528bb38

…o csharp_wrapper

Updating all solution

ddc031a

Removing ARM configuration from desktop solution

370a98e

Added support for building ARM, cleaned up solutions, removed univers…

dc1bf63

…al tests (for now)

Adding Universal Tests back

63230ae

pre10der89 commented Dec 14, 2016

View reviewed changes

facebook-github-bot added the CLA Signed label Dec 22, 2016

Rebasing with master, updating projects with new source files

d1bd845

Updating version on NuGet package spec

2248c61

facebook-github-bot closed this in bf6602e Jan 5, 2017

		@@ -5,42 +5,124 @@ VisualStudioVersion = 14.0.25420.1
		MinimumVisualStudioVersion = 10.0.40219.1

Update C# wrapper to support both desktop and UWP projects... #283

Update C# wrapper to support both desktop and UWP projects... #283

Conversation

pre10der89 commented Dec 14, 2016

facebook-github-bot commented Dec 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rozele Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelMarcey commented Dec 22, 2016

zpao commented Dec 22, 2016

facebook-github-bot commented Dec 22, 2016

matthargett commented Dec 22, 2016

splhack commented Dec 22, 2016

splhack commented Dec 23, 2016

JoelMarcey commented Dec 23, 2016

splhack commented Dec 23, 2016

JoelMarcey commented Dec 24, 2016

rmarinho commented Jan 2, 2017

JoelMarcey commented Jan 3, 2017

pre10der89 commented Jan 3, 2017

emilsjolander commented Jan 4, 2017

emilsjolander commented Jan 4, 2017

rmarinho commented Jan 4, 2017 • edited Loading

JoelMarcey commented Jan 4, 2017

splhack commented Jan 4, 2017

JoelMarcey commented Jan 4, 2017

JoelMarcey commented Jan 5, 2017

rmarinho commented Feb 3, 2017 • edited Loading

pre10der89 commented Feb 3, 2017

emilsjolander commented Feb 3, 2017

rmarinho commented Feb 3, 2017

matthargett commented Feb 3, 2017

matthargett commented Feb 3, 2017

emilsjolander commented Feb 3, 2017

rozele Dec 16, 2016 •

edited

Loading

rmarinho commented Jan 4, 2017 •

edited

Loading

rmarinho commented Feb 3, 2017 •

edited

Loading