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

publish - fix native deps and data folder failing to copy #334

Closed
wants to merge 1 commit into from

Conversation

@jazzay
Copy link

jazzay commented Jan 20, 2019

Here's a solution to #331

  • Adjusts all game projects to support copying native deps properly, and a post publish target to copy the data build folder.
  • Adds macOS to the AllPlatforms.bat

Note: probably we can find a solution to the data copy from within Xenko target files. But not sure best approach there. I tried within Xenko.Core.Assets.CompilerApp.targets, but it did not work.

Also: I switched the Windows game project to using NetCore by default. I believe this will help us track down netcore related problems that affect all platforms, like this issue did. Not sure if you agree! :)

@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Jan 20, 2019

CLA assistant check
All committers have signed the CLA.

@@ -8,6 +8,9 @@
<OutputType>WinExe</OutputType>
<RootNamespace><#= Properties.Namespace #></RootNamespace>

<!-- This resolves native dep publishing -->
<CopyLocalLockFileAssemblies>true</CopyLocalLockFileAssemblies>

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

This will copy all assemblies to output folder (in non-publish mode).
I think dotnet is supposed to work without this (dotnet resolves assemblies from NuGet and native dependencies are loaded from within NuGet).
At least it does under Windows (if I just change linux-x64 to win-x64 and try to run it, everything work out of the box).
Of course if we publish that's another matter, but then it's another fix (CopyData target and the other that needs to be added for native libs that I mentionned in another comment)

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

Sorry, I noticed afterwise CopyData is only for data folder.
We should use a similar Target fix for native libraries. It should probably be located in Xenko.Core\build\Xenko.Core.targets. There is already a bunch of targets like _XenkoSetupNativeLibrariesWindows and _XenkoSetupNativeLibrariesUWP and so on. We probably want a new one _XenkoSetupNativeLibrariesDotNetPublish.

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

Yes I missed that this impacts non-publish scenario. That's not good. Will revisit this.

@@ -2,11 +2,17 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<TargetFramework>net461</TargetFramework>
<!-- We should switch Windows Game to netCore as well. Would help us find xplatform issues quicker -->

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

Let's keep Windows as is.
It's a big breaking change to convert all users Windows project from .NET Framework to .NET Core.
Note that I am not against the concept. However, we'll likely have to do this in a separate PR with some backward compat options (i.e. offer a .NET Core/Framework dropdown when creating the Windows platform).

This comment has been minimized.

Copy link
@Kryptos-FR

Kryptos-FR Jan 20, 2019

Collaborator

Is there a way to have both configuration target generated, and keep Windows as default?

Something like <TargetFrameworks>net461;netcoreapp2.1</TargetFrameworks> should work but I think it builds both targets by default (?), which we might want to avoid.

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

I think it's fine to have two separate projects (either simultaneously with an additional Xenko platform or just a dropdown choice for Windows platform).

Otherwise it would lead to lot of complexity: since the project have some different prorperties, there would be lot of Condition depending on the TargetFramework (that will get destroyed once people edit their project in VS).
And I don't see much advantages over having explicit simple projects that the user can generate/regenerate at will.

So my opinion: let's keep it simple (no MSBuild target-hell like on Xenko dev side), and similar to what VS generates out of the box.

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

Since this is just the template for new projects this should not break existing projects right?

My main concern here is that by not having netcore as the default moving forward we do not see/fix issues on that platform (which is key for Mac/Linux) until later, and increases maintenance burden for us. A question perhaps for the Developer meetup is how to move forward? My vote is to go 100% NetCore on platforms that can support that. There's many advantages to that. And with NetCore3 on the horizon there is improvements for GUI/Window apps.

Regarding short term solution: What about a new Platform in the Project creation that is - NetCore, this could then support all 3 desktop platforms via Multi-targeting. Then we leave existing Windows (.NetFramework) alone. How does that sound? Then user can decide, which or both if they want. I think this what you guys are alluding to above.

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

Sure, I am also all for switching to .NET Core as soon as possible.
However, this probably warrants a discussion on how/when to do it so I would prefer to keep that out of this PR to be able to merge that one quickly.

@@ -135,4 +135,14 @@
</None>
</ItemGroup>
</Target>

<!--Feels like we could do this here, but it does not appear to work-->

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

Please remove that from here (it's a batch file, never going to work)

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

Left this for discussion only to see your thoughts.

@@ -21,4 +24,12 @@
<PackageReference Include="Xenko" Version="<#= Xenko.Assets.XenkoConfig.GetLatestPackageDependency().Version #>" PrivateAssets="contentfiles;analyzers" />
</ItemGroup>

<!-- Would be nice to do this from Xenko targets somehow, but I am unclear how to... -->
<Target Name="CopyData" AfterTargets="Publish">

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

If this works well, we should move it to sources\assets\Xenko.Core.Assets.CompilerApp\build\Xenko.Core.Assets.CompilerApp.targets so that it doesn't pollute user projects.
The idea is to keep the user projects as simple as possible.
Otherwise it will be harder to maintain/update (code is on user side, so harder to upgrade/tweak it esp. if customized by user). Also, Visual Studio default projects referencing Xenko won't work out of the box.

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

Hmm. This comment appears to conflict with your comment above where I tried to add it to the CompilerApp.targets. Maybe I am confused?

This comment has been minimized.

Copy link
@xen2

xen2 Jan 20, 2019

Member

Sorry for the confusion.
There is two targets to write:

  • Copying asset compiler output data (currently called CopyData). This should go in Xenko.Core.Assets.CompilerApp
  • Making .ssdeps (native libs) work. This should go in Xenko.Core

Btw, please make both target names starts with either Xenko or _Xenko (depending if public or not, likely _Xenko in your case)

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

@xen2 regarding ssdeps. Is this a Xenko only thing? I cannot find much in the way of docs about that.

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

After further investigation I can see that this is a custom file format generated from the xenko target file. Will try to figure out how to feed that into a publish copy operation. Any suggestions? Should it be built into an existing C# tool? PackageBuilder for example, new MSBuild Task?

This comment has been minimized.

Copy link
@jazzay

jazzay Jan 20, 2019

Author

Also it feels like we should leverage the built in functionality of nuget/dotnet. Other nuget 3rd party packages accomplish native packaging with no custom code, what is different about Xenko's process?

This comment has been minimized.

Copy link
@xen2

xen2 Jan 21, 2019

Member

Unfortunately there is no built-in functionality.
The point is not to copy but also to set those libs as native lib. In fact, on some platform we don't need to copy them at all.
At the time this was designed, each platform had a wildly different path for that (AndroidNativeLibrary for Android, Copy for UWP but need to choose proper CPU, NativeReference + MtouchExtraArgs for iOS, etc...).
The idea was to hide those differences so that the csproj just add a single XenkoNativeLibrary and it automatically do the proper stuff for each platform.

However, this was done some time ago and it's possible there is easier way to do this (highly doubt it though). Ideally at some point I should write a spec and submit PR for every platform (Xamarin.iOS/Android, dotnet and MSBuild), but I think it would be a lot of work!

xen2 added a commit that referenced this pull request Jan 21, 2019
…ets (fixes #331 and #334) -- thanks to jazzay for the initial implementation
@xen2

This comment has been minimized.

Copy link
Member

xen2 commented Jan 21, 2019

Thanks to your work, I could use some of your research and ideas to make a proper fix.
Closing this PR.

@xen2 xen2 closed this Jan 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.