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

Closed
pre10der89 wants to merge 16 commits from csharp_wrapper into master
pre10der89 commented 2016-12-14 11:24:13 -08:00 (Migrated from github.com)

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.

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.
facebook-github-bot commented 2016-12-14 11:24:24 -08:00 (Migrated from github.com)

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.

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 (Migrated from github.com) reviewed 2016-12-14 11:26:53 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:26:53 -08:00

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.

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 (Migrated from github.com) reviewed 2016-12-14 11:29:47 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:29:47 -08:00

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...

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...
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:30:35 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:30:35 -08:00

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.

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 (Migrated from github.com) reviewed 2016-12-14 11:30:58 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:30:58 -08:00

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

This solution contains all projects including both the universal and desktop tests.
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:31:45 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:31:45 -08:00

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

This is a NETStandard 1.1 PCL project that allows us to build one DLL that targets .NET4.5 and above...
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:32:47 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:32:47 -08:00

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

Removing this xproj in favor of the NETStandard PCL approach...
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:34:09 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:34:09 -08:00

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?

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 (Migrated from github.com) reviewed 2016-12-14 11:34:54 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:34:54 -08:00

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

No idea what version to use... 3.0.0.0 is arbitrary.
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:35:24 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:35:24 -08:00

NETStandard 1.1 is the minimum that can be used with DLLImport

NETStandard 1.1 is the minimum that can be used with DLLImport
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:35:46 -08:00
@@ -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">
pre10der89 (Migrated from github.com) commented 2016-12-14 11:35:46 -08:00

Added ARM configuration

Added ARM configuration
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:36:10 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:36:10 -08:00

Secret flag allowing Visual Studio to build ARM

Secret flag allowing Visual Studio to build ARM
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:36:41 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:36:41 -08:00

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

Output directory to match pattern of the c# wrapper and tests
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:37:22 -08:00
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
pre10der89 (Migrated from github.com) commented 2016-12-14 11:37:22 -08:00

Added version resource to Yoga.DLL

Added version resource to Yoga.DLL
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:38:19 -08:00
@@ -0,0 +1,38 @@
<?xml version="1.0" encoding="utf-8"?>
pre10der89 (Migrated from github.com) commented 2016-12-14 11:38:19 -08:00

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

Nuget spec file that will create a native only package for the Yoga.DLL... This follows the pattern used by sqlite.native
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:39:08 -08:00
@@ -0,0 +22,4 @@
<file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
<!-- lib -->
<file src="_._" target="lib\netstandard1.0"/>
pre10der89 (Migrated from github.com) commented 2016-12-14 11:39:08 -08:00

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

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
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:40:40 -08:00
@@ -0,0 +1,36 @@
<?xml version="1.0" encoding="utf-8" ?>
pre10der89 (Migrated from github.com) commented 2016-12-14 11:40:40 -08:00

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.

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 (Migrated from github.com) reviewed 2016-12-14 11:41:18 -08:00
@@ -0,0 +1,46 @@
<?xml version="1.0" encoding="utf-8"?>
pre10der89 (Migrated from github.com) commented 2016-12-14 11:41:18 -08:00

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

Nuget spec for the C# wrapper and its associated native dlls... This is uses the sqllite style packaging...
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:42:32 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:42:32 -08:00

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...

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 (Migrated from github.com) reviewed 2016-12-14 11:44:00 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:44:00 -08:00

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.

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.
pre10der89 (Migrated from github.com) reviewed 2016-12-14 11:44:59 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-14 11:44:59 -08:00

These are a subset of the generated tests... They have been converted to MSTest... Again, the reason for this is that the generated tests target NUnit 2.6.4 and that is not supported in Universal

These are a subset of the generated tests... They have been converted to MSTest... Again, the reason for this is that the generated tests target NUnit 2.6.4 and that is not supported in Universal
emilsjolander commented 2016-12-15 03:24:00 -08:00 (Migrated from github.com)

Why did you include image assets here? I think those should be removed. For the other stuff I'll delegate to @splhack or @mattpodwysocki who knows much more about the .NET stack.

Why did you include image assets here? I think those should be removed. For the other stuff I'll delegate to @splhack or @mattpodwysocki who knows much more about the .NET stack.
mattpodwysocki commented 2016-12-15 09:55:24 -08:00 (Migrated from github.com)

@emilsjolander those image assets are mainly for the test runner for the UWP tests, so unfortunately you have to install the app, and those are the icons it uses for the tile, taskbar icon, etc. It's best to keep them for now unless you want to swap them out with any official Facebook icons.

We have the same thing with our tests in ReactNative Windows as well

@emilsjolander those image assets are mainly for the test runner for the UWP tests, so unfortunately you have to install the app, and those are the icons it uses for the tile, taskbar icon, etc. It's best to keep them for now unless you want to swap them out with any official Facebook icons. We have the same thing with our tests in [ReactNative Windows](https://github.com/ReactWindows/react-native-windows/tree/master/ReactWindows/ReactNative.Tests/Assets) as well
emilsjolander commented 2016-12-15 13:31:10 -08:00 (Migrated from github.com)

What license are the icons under and where do they come from?

What license are the icons under and where do they come from?
pre10der89 commented 2016-12-15 13:45:35 -08:00 (Migrated from github.com)

The icons come from the default "Unit Test App (Universal Windows)" template in Visual Studio 2015... As Matt pointed out they are part of the Unit Test Runner for the Universal Window Unit tests... Since UWP requires an app container they create an application with a GUI to run the tests.

The icons come from the default "Unit Test App (Universal Windows)" template in Visual Studio 2015... As Matt pointed out they are part of the Unit Test Runner for the Universal Window Unit tests... Since UWP requires an app container they create an application with a GUI to run the tests.
matthargett (Migrated from github.com) reviewed 2016-12-15 14:54:54 -08:00
matthargett (Migrated from github.com) commented 2016-12-15 14:54:53 -08:00

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.

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.
matthargett commented 2016-12-15 15:44:51 -08:00 (Migrated from github.com)

Looks like the Travis CI is failing for unrelated reasons, since no files related were modified in this PR:

BUILT 14/26 JOBS 0.2s //:yoga#iphonesimulator-x86_64,preprocess-Yoga.c.i110b516b
YogaKit/Tests/YogaKitTests.m:12:9: fatal error: 'UIView+Yoga.h' file not found
#import "UIView+Yoga.h"
        ^
1 error generated.
Looks like the Travis CI is failing for unrelated reasons, since no files related were modified in this PR: ``` BUILT 14/26 JOBS 0.2s //:yoga#iphonesimulator-x86_64,preprocess-Yoga.c.i110b516b YogaKit/Tests/YogaKitTests.m:12:9: fatal error: 'UIView+Yoga.h' file not found #import "UIView+Yoga.h" ^ 1 error generated. ```
splhack commented 2016-12-15 23:28:18 -08:00 (Migrated from github.com)

What do you guys think about this directory hierarchy?

csharp/Windows [NEW]
csharp/Windows/Facebook.Yoga.Desktop.sln
csharp/Windows/...

csharp/Xamarin.iOS [New]
csharp/Xamarin.iOS/Facebook.Yoga.iOS/...
csharp/Xamarin.iOS/...

csharp/Xamarin.Mac [New]
csharp/Xamarin.Mac/Facebook.Yoga.Mac/...
csharp/Xamarin.Mac/...

csharp/Xamarin.Android [New]
csharp/Xamarin.Android/Facebook.Yoga.Android/...
csharp/Xamarin.Android/...

csharp/Unity [New]
csharp/Unity/Facebook.Yoga.Unity/...
csharp/Unity/...

csharp/Facebook.Yoga (plain C# code)
csharp/tests (plain C# test)
csharp/Yoga (native code)
What do you guys think about this directory hierarchy? csharp/Windows [NEW] csharp/Windows/Facebook.Yoga.Desktop.sln csharp/Windows/... csharp/Xamarin.iOS [New] csharp/Xamarin.iOS/Facebook.Yoga.iOS/... csharp/Xamarin.iOS/... csharp/Xamarin.Mac [New] csharp/Xamarin.Mac/Facebook.Yoga.Mac/... csharp/Xamarin.Mac/... csharp/Xamarin.Android [New] csharp/Xamarin.Android/Facebook.Yoga.Android/... csharp/Xamarin.Android/... csharp/Unity [New] csharp/Unity/Facebook.Yoga.Unity/... csharp/Unity/... csharp/Facebook.Yoga (plain C# code) csharp/tests (plain C# test) csharp/Yoga (native code)
emilsjolander commented 2016-12-15 23:39:35 -08:00 (Migrated from github.com)

@matthargett yup, that test is now passing on master. sorry about that.

@splhack That looks good to me. However I'll leave it to people more familiar with .NET to decide.

@matthargett yup, that test is now passing on master. sorry about that. @splhack That looks good to me. However I'll leave it to people more familiar with .NET to decide.
chamons commented 2016-12-16 09:05:14 -08:00 (Migrated from github.com)

Only comment is do you to describe what "flavor" of windows that is (UWP, etc)?

Only comment is do you to describe what "flavor" of windows that is (UWP, etc)?
splhack commented 2016-12-16 09:50:10 -08:00 (Migrated from github.com)

@chamons I'm not so familiar with Windows platforms. maybe csharp/UWP or csharp/Windows/Facebook.Yoga.UWP?

@chamons I'm not so familiar with Windows platforms. maybe `csharp/UWP` or `csharp/Windows/Facebook.Yoga.UWP`?
rozele (Migrated from github.com) reviewed 2016-12-16 10:19:55 -08:00
rozele (Migrated from github.com) commented 2016-12-16 10:19:55 -08:00

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?

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?
rozele (Migrated from github.com) reviewed 2016-12-16 10:30:13 -08:00
rozele (Migrated from github.com) commented 2016-12-16 10:30:13 -08:00

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


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

Are you building the NuGet package from the two separated solutions or just from Facebook.Yoga.sln? --- In reply to: [92469842](https://github.com/facebook/yoga/pull/283#discussion_r92469842) [](ancestors = 92469842)
rozele (Migrated from github.com) reviewed 2016-12-16 10:33:10 -08:00
@@ -0,0 +22,4 @@
<file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
<!-- lib -->
<file src="_._" target="lib\netstandard1.0"/>
rozele (Migrated from github.com) commented 2016-12-16 10:33:10 -08:00

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


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

This seems like magic, do you have any documentation on why this works? --- In reply to: [92471862](https://github.com/facebook/yoga/pull/283#discussion_r92471862) [](ancestors = 92471862)
rozele (Migrated from github.com) reviewed 2016-12-16 10:38:07 -08:00
rozele (Migrated from github.com) commented 2016-12-16 10:38:07 -08:00

NuGet [](start = 9, length = 5)

Just curious, is it common to commit this and nuget.exe to the repo? In the past, when I've created projects like this published on NuGet, I tend to create a separate (private) management repo with a submodule for the open source project. That way I can keep private keys for code signing, scripts like this one, and NuGet.exe there.

>NuGet [](start = 9, length = 5) Just curious, is it common to commit this and nuget.exe to the repo? In the past, when I've created projects like this published on NuGet, I tend to create a separate (private) management repo with a submodule for the open source project. That way I can keep private keys for code signing, scripts like this one, and NuGet.exe there.
rozele (Migrated from github.com) reviewed 2016-12-16 10:40:53 -08:00
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
rozele (Migrated from github.com) commented 2016-12-16 10:40:53 -08:00

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


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

What are Yoga.rc and resource.h used for? --- In reply to: [92471469](https://github.com/facebook/yoga/pull/283#discussion_r92471469) [](ancestors = 92471469)
rozele (Migrated from github.com) reviewed 2016-12-16 10:42:21 -08:00
rozele (Migrated from github.com) commented 2016-12-16 10:42:21 -08:00
  1. [](start = 34, length = 2)

Should we be building this individually for each target platform?

>8. [](start = 34, length = 2) Should we be building this individually for each target platform?
matthargett (Migrated from github.com) reviewed 2016-12-16 11:35:28 -08:00
matthargett (Migrated from github.com) commented 2016-12-16 11:35:28 -08:00

We weren't sure until we tested, but the UWP build is able to delegate to native library compiled for this 8.x platform just fine. So, it looks like a single project and target version works fine.

We weren't sure until we tested, but the UWP build is able to delegate to native library compiled for this 8.x platform just fine. So, it looks like a single project and target version works fine.
chamons commented 2016-12-16 11:41:28 -08:00 (Migrated from github.com)

@splhack As the Xamarin.Mac guy, I know very little about Windows myself anymore

@splhack As the Xamarin.Mac guy, I know very little about Windows myself anymore
dhaligas commented 2016-12-16 11:56:45 -08:00 (Migrated from github.com)

@splhack excited to see this land on Xamarin. Would definitely like to see this with Xamarin.Forms as well

@splhack excited to see this land on Xamarin. Would definitely like to see this with Xamarin.Forms as well
pre10der89 (Migrated from github.com) reviewed 2016-12-16 13:54:10 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-16 13:54:10 -08:00

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.

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.
pre10der89 (Migrated from github.com) reviewed 2016-12-16 13:56:31 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-16 13:56:31 -08:00

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 :)

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 :)
pre10der89 (Migrated from github.com) reviewed 2016-12-16 13:57:49 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-16 13:57:49 -08:00

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...

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 (Migrated from github.com) reviewed 2016-12-16 13:58:30 -08:00
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
pre10der89 (Migrated from github.com) commented 2016-12-16 13:58:30 -08:00

They apply a version to the native DLL

They apply a version to the native DLL
chamons (Migrated from github.com) reviewed 2016-12-16 13:58:56 -08:00
chamons (Migrated from github.com) commented 2016-12-16 13:58:56 -08:00

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

I know nothing about BUCK, but can't it invoke arbitrary command line tools? xbuild/msbuild?
pre10der89 (Migrated from github.com) reviewed 2016-12-16 15:00:57 -08:00
@@ -0,0 +22,4 @@
<file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
<!-- lib -->
<file src="_._" target="lib\netstandard1.0"/>
pre10der89 (Migrated from github.com) commented 2016-12-16 15:00:57 -08:00

@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...

@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](https://github.com/fsprojects/Paket/issues/193) that sort of discusses it...
pre10der89 (Migrated from github.com) reviewed 2016-12-16 15:16:37 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-16 15:16:37 -08:00

@rozele I have seen repositories commit the nuget.exe and I've seen others that do not... CIs like AppVeyor have provisions for building packages... I've also seen many flavors of the approach you mention... I would rather NOT have the nuget.exe or powershell script here so I'll remove them from the PR...

@rozele I have seen repositories commit the nuget.exe and I've seen others that do not... CIs like AppVeyor have provisions for building packages... I've also seen many flavors of the approach you mention... I would rather NOT have the nuget.exe or powershell script here so I'll remove them from the PR...
pre10der89 (Migrated from github.com) reviewed 2016-12-16 15:22:09 -08:00
pre10der89 (Migrated from github.com) commented 2016-12-16 15:22:09 -08:00

@rozele I started with the approach of creating two separate Native DLLs targeting 8.1 or the UWP CRT... This created additional complexity in the solution and ultimately the nuget package... From all my testing it appears that the 8.1 targeted native build works fine with both Desktop and Universal in both release and debug configurations... My tests were not exhaustive across the entire API surface area and I did not take into account any performance differences...

@rozele I started with the approach of creating two separate Native DLLs targeting 8.1 or the UWP CRT... This created additional complexity in the solution and ultimately the nuget package... From all my testing it appears that the 8.1 targeted native build works fine with both Desktop and Universal in both release and debug configurations... My tests were not exhaustive across the entire API surface area and I did not take into account any performance differences...
pre10der89 commented 2016-12-16 15:28:25 -08:00 (Migrated from github.com)

@splhack @dhaligas We are definitely interested in the Linux platform and Xamarin as an extension to React Native... Since the csproj is targeting NETStandard we don't see why it wouldn't be compatible with Mono, Xamarin, or any other platform that supports NETStandard. I'm not an expert in those other platforms so it would be helpful if we could test this approach on those platforms...

@splhack @dhaligas We are definitely interested in the Linux platform and Xamarin as an extension to React Native... Since the csproj is targeting NETStandard we don't see why it wouldn't be compatible with Mono, Xamarin, or any other platform that supports NETStandard. I'm not an expert in those other platforms so it would be helpful if we could test this approach on those platforms...
pre10der89 commented 2016-12-16 15:30:48 -08:00 (Migrated from github.com)

@splhack I agree the directory structure might be improve to support all the different flavors of .NET... It's probably something we could work on in a future PR? There are new techniques for creating cross-platform NETStandard libraries that we could surely employ to create an effective code structure...

@splhack I agree the directory structure might be improve to support all the different flavors of .NET... It's probably something we could work on in a future PR? There are new techniques for creating cross-platform NETStandard libraries that we could surely employ to create an effective code structure...
facebook-github-bot commented 2016-12-16 16:03:16 -08:00 (Migrated from github.com)

@pre10der89 updated the pull request - view changes

@pre10der89 updated the pull request - [view changes](https://github.com/facebook/yoga/pull/283/files/63230ae46ab182651b193760d879b38de04b0702..c5ab9637f46fb4a2afe29f66d95fc2cc0b63b9d1)
splhack commented 2016-12-16 16:10:19 -08:00 (Migrated from github.com)

@pre10der89 Do you mind to move Windows specific files to csharp/Windows directory? It's difficult?

@pre10der89 Do you mind to move Windows specific files to `csharp/Windows` directory? It's difficult?
pre10der89 commented 2016-12-16 18:19:19 -08:00 (Migrated from github.com)

@splhack It would be simple enough to move the code into a Windows folder... I would go even
further and suggest a folder structure as follows:

\NuGet
\Shared
\Yoga
\Facebook.Yoga
Facebook.Yoga.Shared.shproj
Facebook.Yoga.Shared.projitems
*.cs
\Facebook.Yoga.Tests
Facebook.Yoga.Shared.Tests.projitems
Facebook.Yoga.Shared.Tests.shproj
*.cs
\Unity
\Windows
\Build
\Facebook.Yoga
\Facebook.Yoga.Desktop.Tests
\Facebook.Yoga.Universal.Tests
Facebook.Yoga.sln
Facebook.Yoga.Desktop.sln
Facebook.Yoga.Universal.sln
\Xamarin.Android
\Xamarin.IOS
\Xamarin.Mac

The shared code is encapsulated into a shared folder and we use shared projects that are supported by both visual studio and xamarin and perhaps other .NET projects... I recommend
shared projects so that we don't have to have maintain links in the Windows projects. Using
the shared projects themselves would be optional for any platform. The new "Shared" folder and the rename of the "tests" to "Facebook.Yoga.Tests" would break all the builds so indivdiual contributors for the other platforms would need to fix their structure...

It is beyond the scope of this PR, but there may be a better solution in the future that would involve using a Mult-targeted .NET Core project (xproj) that would allow us to build all our concerns from the same project... I don't currently have enough experience with this approach to know whether it will work for all parties. It is a future consideration that would allow us to collapse much of the complexity in the directory structure...

Thoughts?

@splhack It would be simple enough to move the code into a Windows folder... I would go even further and suggest a folder structure as follows: \NuGet \Shared \Yoga \Facebook.Yoga Facebook.Yoga.Shared.shproj Facebook.Yoga.Shared.projitems *.cs \Facebook.Yoga.Tests Facebook.Yoga.Shared.Tests.projitems Facebook.Yoga.Shared.Tests.shproj *.cs \Unity \Windows \Build \Facebook.Yoga \Facebook.Yoga.Desktop.Tests \Facebook.Yoga.Universal.Tests Facebook.Yoga.sln Facebook.Yoga.Desktop.sln Facebook.Yoga.Universal.sln \Xamarin.Android \Xamarin.IOS \Xamarin.Mac The shared code is encapsulated into a shared folder and we use shared projects that are supported by both visual studio and xamarin and perhaps other .NET projects... I recommend shared projects so that we don't have to have maintain links in the Windows projects. Using the shared projects themselves would be optional for any platform. The new "Shared" folder and the rename of the "tests" to "Facebook.Yoga.Tests" would break all the builds so indivdiual contributors for the other platforms would need to fix their structure... It is beyond the scope of this PR, but there may be a better solution in the future that would involve using a Mult-targeted .NET Core project (xproj) that would allow us to build all our concerns from the same project... I don't currently have enough experience with this approach to know whether it will work for all parties. It is a future consideration that would allow us to collapse much of the complexity in the directory structure... Thoughts?
rozele (Migrated from github.com) reviewed 2016-12-19 10:40:20 -08:00
rozele (Migrated from github.com) commented 2016-12-19 10:40:20 -08:00

The performance differences is what I'm primarily concerned with. We can start with this and look into optimizations after. If we do need to separate, it shouldn't be too complex, you'll just have to add a bunch of build configurations to the solution.


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

The performance differences is what I'm primarily concerned with. We can start with this and look into optimizations after. If we do need to separate, it shouldn't be too complex, you'll just have to add a bunch of build configurations to the solution. --- In reply to: [92902849](https://github.com/facebook/yoga/pull/283#discussion_r92902849) [](ancestors = 92902849)
rozele commented 2016-12-19 10:42:56 -08:00 (Migrated from github.com)

:shipit:

:shipit:
mattpodwysocki commented 2016-12-19 12:14:30 -08:00 (Migrated from github.com)

@pre10der89 before finalizing, please remove the icons for now and Facebook will provide their own

@pre10der89 before finalizing, please remove the icons for now and Facebook will provide their own
rozele commented 2016-12-19 12:27:57 -08:00 (Migrated from github.com)

Is there anything wrong with providing the stub icons that are generated by Visual Studio until they can be replaced?

Is there anything wrong with providing the stub icons that are generated by Visual Studio until they can be replaced?
emilsjolander commented 2016-12-19 12:59:19 -08:00 (Migrated from github.com)

@rozele It's easier to provide icons (or just colored squares) which we make our own to avoid any licensing questions / issues.

@rozele It's easier to provide icons (or just colored squares) which we make our own to avoid any licensing questions / issues.
pre10der89 commented 2016-12-19 13:54:16 -08:00 (Migrated from github.com)

@emilsjolander I will erase the contents of the icons in the Assets folder... They will appear as solid black... Will this work?

@emilsjolander I will erase the contents of the icons in the Assets folder... They will appear as solid black... Will this work?
pre10der89 commented 2016-12-19 14:04:26 -08:00 (Migrated from github.com)

@splhack I will be uploading a change that will isolate all Windows related code into a windows folder... I will leave the three shared folders as they are (e.g. I will not place them into a "shared" folder)...

Here is the layout... Let me know if there is anything you'd like to see different...

csharp\Facebook.Yoga
csharp\Facebook.Yoga\Facebook.Yoga.Shared.projitems
csharp\Facebook.Yoga\Facebook.Yoga.Shared.shproj
csharp\Facebook.Yoga*.cs

csharp\NuGet\

csharp\tests\Facebook.Yoga
csharp\tests\Facebook.Yoga\Facebook.Yoga.Shared.Tests.projitems
csharp\tests\Facebook.Yoga\Facebook.Yoga.Shared.Tests.shproj
csharp\tests\Facebook.Yoga*.cs

csharp\Windows\Build
csharp\Windows\Facebook.Yoga
csharp\Windows\Facebook.Yoga.Desktop.Tests
csharp\Windows\Facebook.Yoga.Universal.Tests
csharp\Windows\Facebook.Yoga.sln
csharp\Windows\Facebook.Yoga.Desktop.sln
csharp\Windows\Facebook.Yoga.Universal.sln

csharp\Yoga\

@splhack I will be uploading a change that will isolate all Windows related code into a windows folder... I will leave the three shared folders as they are (e.g. I will not place them into a "shared" folder)... Here is the layout... Let me know if there is anything you'd like to see different... csharp\Facebook.Yoga\ csharp\Facebook.Yoga\Facebook.Yoga.Shared.projitems csharp\Facebook.Yoga\Facebook.Yoga.Shared.shproj csharp\Facebook.Yoga\*.cs csharp\NuGet\ csharp\tests\Facebook.Yoga\ csharp\tests\Facebook.Yoga\Facebook.Yoga.Shared.Tests.projitems csharp\tests\Facebook.Yoga\Facebook.Yoga.Shared.Tests.shproj csharp\tests\Facebook.Yoga\*.cs csharp\Windows\Build\ csharp\Windows\Facebook.Yoga\ csharp\Windows\Facebook.Yoga.Desktop.Tests\ csharp\Windows\Facebook.Yoga.Universal.Tests\ csharp\Windows\Facebook.Yoga.sln csharp\Windows\Facebook.Yoga.Desktop.sln csharp\Windows\Facebook.Yoga.Universal.sln csharp\Yoga\
facebook-github-bot commented 2016-12-19 14:11:59 -08:00 (Migrated from github.com)

@pre10der89 updated the pull request - view changes

@pre10der89 updated the pull request - [view changes](https://github.com/facebook/yoga/pull/283/files/c5ab9637f46fb4a2afe29f66d95fc2cc0b63b9d1..9903a20cbad8965bcb27d80dffbdbd6874879b5a)
emilsjolander commented 2016-12-20 00:26:16 -08:00 (Migrated from github.com)

@pre10der89 That sounds good! Thanks

@pre10der89 That sounds good! Thanks
pre10der89 commented 2016-12-20 10:01:24 -08:00 (Migrated from github.com)

@emilsjolander Have you received our signed CLA? I don't see the CLA Signed tag on this PR...

@emilsjolander Have you received our signed CLA? I don't see the CLA Signed tag on this PR...
emilsjolander commented 2016-12-21 00:16:08 -08:00 (Migrated from github.com)

@pre10der89 looking into it

@pre10der89 looking into it
JoelMarcey commented 2016-12-21 07:59:20 -08:00 (Migrated from github.com)

@pre10der89 Hi! Did you sign an individual CLA or are you through a corporate CLA? And did you sign online or sign/scan via email? Thanks!

@pre10der89 Hi! Did you sign an individual CLA or are you through a corporate CLA? And did you sign online or sign/scan via email? Thanks!
splhack commented 2016-12-21 09:52:30 -08:00 (Migrated from github.com)

@pre10der89 can you rebase this PR to the master? For example, Native.cs YGNodeInit is already gone in the master.

@pre10der89 can you rebase this PR to the master? For example, Native.cs `YGNodeInit` is already gone in the master.
pre10der89 commented 2016-12-21 10:45:20 -08:00 (Migrated from github.com)

@JoelMarcey My director signed the corporate CLA... I believe the CLA was signed online... @matthargett

@JoelMarcey My director signed the corporate CLA... I believe the CLA was signed online... @matthargett
pre10der89 commented 2016-12-21 10:51:12 -08:00 (Migrated from github.com)

@splhack Thank you for pointing that out... That was a merge conflict mistake... I have removed the YGNodeInit from my PR...

@splhack Thank you for pointing that out... That was a merge conflict mistake... I have removed the YGNodeInit from my PR...
JoelMarcey commented 2016-12-22 07:47:15 -08:00 (Migrated from github.com)

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

@pre10der89 What is the company that the CLA would be under?
zpao commented 2016-12-22 13:03:27 -08:00 (Migrated from github.com)

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).

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 commented 2016-12-22 13:08:29 -08:00 (Migrated from github.com)

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

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
matthargett commented 2016-12-22 14:06:00 -08:00 (Migrated from github.com)

ETA for merge? We'd like this updated package published so we can integrate it into the next React-Native-Windows release. https://github.com/ReactWindows/react-native-windows/pull/941

ETA for merge? We'd like this updated package published so we can integrate it into the next React-Native-Windows release. https://github.com/ReactWindows/react-native-windows/pull/941
splhack commented 2016-12-22 14:21:17 -08:00 (Migrated from github.com)

@matthargett maybe today (or tomorrow)!

@matthargett maybe today (or tomorrow)!
splhack commented 2016-12-22 23:09:29 -08:00 (Migrated from github.com)

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

@matthargett unfortunately we're having some tool issue now, and holiday season, maybe it'll take a bit longer...
JoelMarcey commented 2016-12-23 08:25:13 -08:00 (Migrated from github.com)

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

@matthargett I am going to try to resolve the problem for this particular issue sometime between today and the end of the weekend.
splhack commented 2016-12-23 09:40:05 -08:00 (Migrated from github.com)

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

@pre10der89 do you mind to rebase this PR to the master again? #297 changed Native.cs and project.json.
JoelMarcey commented 2016-12-24 08:15:16 -08:00 (Migrated from github.com)

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

@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 commented 2017-01-02 06:37:14 -08:00 (Migrated from github.com)

Any progress on this?

Any progress on this?
JoelMarcey commented 2017-01-03 07:04:55 -08:00 (Migrated from github.com)

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

Once this pull request is rebased, I should be able to get this synced up manually.
pre10der89 commented 2017-01-03 10:40:54 -08:00 (Migrated from github.com)

@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...

@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 commented 2017-01-04 04:04:07 -08:00 (Migrated from github.com)

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

The failing tests are unrelated. Another PR is currently working on solving those. @JoelMarcey go ahead and import
emilsjolander commented 2017-01-04 04:40:07 -08:00 (Migrated from github.com)

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

Travis should be fixed now. If you rebase all tests should pass.
rmarinho commented 2017-01-04 05:01:25 -08:00 (Migrated from github.com)

@pre10der89 can you rebase so @JoelMarcey can import

@pre10der89 can you rebase so @JoelMarcey can import
JoelMarcey commented 2017-01-04 07:07:31 -08:00 (Migrated from github.com)

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

@rmarinho This was rebased yesterday by @pre10der89. Are we waiting for another rebase or can I go on and try to manually import?
splhack commented 2017-01-04 09:26:42 -08:00 (Migrated from github.com)

@JoelMarcey we can go on!

@JoelMarcey we can go on!
JoelMarcey commented 2017-01-04 10:47:48 -08:00 (Migrated from github.com)

D4381455 internally

D4381455 internally
JoelMarcey commented 2017-01-04 17:03:27 -08:00 (Migrated from github.com)

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

Looks like this is good now. Just having @emilsjolander look at the internal diff for a final check and then we can ship.
rmarinho commented 2017-02-03 09:23:22 -08:00 (Migrated from github.com)

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

Hey @pre10der89 can you email me to try some packages ? me at ruimarinho.net
pre10der89 commented 2017-02-03 11:31:30 -08:00 (Migrated from github.com)

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

@rmarinho The package has been published to NuGet... Search for Facebook.Yoga...
emilsjolander commented 2017-02-03 11:35:43 -08:00 (Migrated from github.com)

@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.

@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 commented 2017-02-03 12:18:59 -08:00 (Migrated from github.com)

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 ..

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 commented 2017-02-03 12:33:42 -08:00 (Migrated from github.com)

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.

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 commented 2017-02-03 12:38:14 -08:00 (Migrated from github.com)

@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 :)

@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 commented 2017-02-03 12:42:42 -08:00 (Migrated from github.com)

@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

@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

Pull request closed

Sign in to join this conversation.
No description provided.