Update C# wrapper to support both desktop and UWP projects... #283
Reference in New Issue
Block a user
No description provided.
Delete Branch "csharp_wrapper"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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.
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.
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.
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...
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.
This solution contains all projects including both the universal and desktop tests.
This is a NETStandard 1.1 PCL project that allows us to build one DLL that targets .NET4.5 and above...
Removing this xproj in favor of the NETStandard PCL approach...
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?
No idea what version to use... 3.0.0.0 is arbitrary.
NETStandard 1.1 is the minimum that can be used with DLLImport
@@ -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
Secret flag allowing Visual Studio to build ARM
Output directory to match pattern of the c# wrapper and tests
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
Added version resource to Yoga.DLL
@@ -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
@@ -0,0 +22,4 @@
<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
@@ -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.
@@ -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...
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...
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.
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
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.
@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
What license are the icons under and where do they come from?
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.
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.
Looks like the Travis CI is failing for unrelated reasons, since no files related were modified in this PR:
What do you guys think about this directory hierarchy?
@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.
Only comment is do you to describe what "flavor" of windows that is (UWP, etc)?
@chamons I'm not so familiar with Windows platforms. maybe
csharp/UWP
orcsharp/Windows/Facebook.Yoga.UWP
?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?
Are you building the NuGet package from the two separated solutions or just from Facebook.Yoga.sln?
In reply to: 92469842 [](ancestors = 92469842)
@@ -0,0 +22,4 @@
<file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
<!-- lib -->
<file src="_._" target="lib\netstandard1.0"/>
This seems like magic, do you have any documentation on why this works?
In reply to: 92471862 [](ancestors = 92471862)
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.
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
What are Yoga.rc and resource.h used for?
In reply to: 92471469 [](ancestors = 92471469)
Should we be building this individually for each target platform?
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.
@splhack As the Xamarin.Mac guy, I know very little about Windows myself anymore
@splhack excited to see this land on Xamarin. Would definitely like to see this with Xamarin.Forms as well
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'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...
@@ -0,0 +1,14 @@
//{{NO_DEPENDENCIES}}
They apply a version to the native DLL
I know nothing about BUCK, but can't it invoke arbitrary command line tools? xbuild/msbuild?
@@ -0,0 +22,4 @@
<file src="Facebook.Yoga.Native.targets" target="build\netstandard1.0"/>
<!-- lib -->
<file src="_._" target="lib\netstandard1.0"/>
@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 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 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...
@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 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...
@pre10der89 updated the pull request - view changes
@pre10der89 Do you mind to move Windows specific files to
csharp/Windows
directory? It's difficult?@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?
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)
:shipit:
@pre10der89 before finalizing, please remove the icons for now and Facebook will provide their own
Is there anything wrong with providing the stub icons that are generated by Visual Studio until they can be replaced?
@rozele It's easier to provide icons (or just colored squares) which we make our own to avoid any licensing questions / issues.
@emilsjolander I will erase the contents of the icons in the Assets folder... They will appear as solid black... Will this work?
@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\
@pre10der89 updated the pull request - view changes
@pre10der89 That sounds good! Thanks
@emilsjolander Have you received our signed CLA? I don't see the CLA Signed tag on this PR...
@pre10der89 looking into it
@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 can you rebase this PR to the master? For example, Native.cs
YGNodeInit
is already gone in the master.@JoelMarcey My director signed the corporate CLA... I believe the CLA was signed online... @matthargett
@splhack Thank you for pointing that out... That was a merge conflict mistake... I have removed the YGNodeInit from my PR...
@pre10der89 What is the company that the CLA would be under?
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).
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!
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
@matthargett maybe today (or tomorrow)!
@matthargett unfortunately we're having some tool issue now, and holiday season, maybe it'll take a bit longer...
@matthargett I am going to try to resolve the problem for this particular issue sometime between today and the end of the weekend.
@pre10der89 do you mind to rebase this PR to the master again? #297 changed Native.cs and project.json.
@pre10der89 Once you rebase this pull request, I have an idea on how we can get this imported internally successfully, and thus landed. Thanks!
Any progress on this?
Once this pull request is rebased, I should be able to get this synced up manually.
@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...
The failing tests are unrelated. Another PR is currently working on solving those. @JoelMarcey go ahead and import
Travis should be fixed now. If you rebase all tests should pass.
@pre10der89 can you rebase so @JoelMarcey can import
@rmarinho This was rebased yesterday by @pre10der89. Are we waiting for another rebase or can I go on and try to manually import?
@JoelMarcey we can go on!
D4381455 internally
Looks like this is good now. Just having @emilsjolander look at the internal diff for a final check and then we can ship.
Hey @pre10der89 can you email me to try some packages ? me at ruimarinho.net
@rmarinho The package has been published to NuGet... Search for Facebook.Yoga...
@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.
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 ..
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.
@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 :)
@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