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

One NuGet for all OSs #19

Open
bddckr opened this issue Jul 25, 2020 · 8 comments
Open

One NuGet for all OSs #19

bddckr opened this issue Jul 25, 2020 · 8 comments

Comments

@bddckr
Copy link
Contributor

bddckr commented Jul 25, 2020

Describe the feature you'd like

Offer one NuGet package, that is multi-targeted and contains the necessary assemblies to support all supported OSs. Then additionally the <OsName>Application.Init API can be simplified to do the right call based on a runtime check. This would replace the current packages as it can be a full replacement for consumers of SpiderEye.

Reason for this feature request

Currently projects that use SpiderEye on more than one target OS have to be developed with N+1 projects, where N is the number of the OSs. The reason for that is the fact that SpiderEye currently ships support for each OS in a separate NuGet. The remaining project acts as the "core" project that defines most of the logic, while the other projects all just configure the app on startup via the correct <OsName>Application API for that OS.

With this change a user of SpiderEye would only add a single NuGet package to their project and call a single initialization method. Besides a bigger NuGet package for developers to download this would not result in unnecessary assembly additions for a specific target OS.

Besides simplification for consumers this change would most likely also

  • remove the need for the templates, as long as we can assume the consumer will not do platform-specific calls in most cases (otherwise they can still set up one project per OS themselves, though).
  • allow the consumer to publish a single project for each target OS simply by publishing with the corresponding runtime identifier.
@bddckr
Copy link
Contributor Author

bddckr commented Jul 25, 2020

I've not done this before myself yet, and most examples I know about only call into different native libraries for each platform, but all of them share a common API already.

So I'm wondering if this is possible for SpiderEye - there needs to some smart handling/loading of a specific OS's .dll or the SpiderEye source needs to change to merge all OS-specific source projects and do some good old #ifdef? The latter makes the development experience worse for maintaining SpiderEye for sure, so let's not aim for that 😅

@bddckr
Copy link
Contributor Author

bddckr commented Jul 26, 2020

The following .csproj does add the various existing assemblies into a single NuGet package, which seems to work. I named it SpiderEye.Packaging, so that's why the AssemblyName gets adjusted before the import of SpiderEye.Shared.proj, which then results in a package just called Bildstein.SpiderEye.

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <AssemblyName>$(MSBuildProjectName.Replace('.Packaging', ''))</AssemblyName>
  </PropertyGroup>
  <Import Project="..\Shared\SpiderEye.Shared.proj" />

  <PropertyGroup>
    <TargetFramework>netcoreapp3.0</TargetFramework>
    <IncludeBuildOutput>false</IncludeBuildOutput>
    <DisableTransitiveFrameworkReferences>true</DisableTransitiveFrameworkReferences>
  </PropertyGroup>

  <ItemGroup>
    <ProjectReference Include="..\**\*.csproj" />
    <ProjectReference Remove="$(MSBuildProjectFullPath)" />
  </ItemGroup>

  <ItemGroup>
    <None Include="$(OutputPath)**/*.Core.*" Pack="true" PackagePath="ref/$(TargetFramework);runtimes/linux-x64/lib/$(TargetFramework);runtimes/osx-x64/lib/$(TargetFramework);runtimes/win/lib/$(TargetFramework)" />
    <None Include="$(OutputPath)**/*.Linux.*" Pack="true" PackagePath="runtimes/linux-x64/lib/$(TargetFramework)" />
    <None Include="$(OutputPath)**/*.Mac.*" Pack="true" PackagePath="runtimes/osx-x64/lib/$(TargetFramework)" />
    <None Include="$(OutputPath)**/*.Windows.*" Pack="true" PackagePath="runtimes/win/lib/$(TargetFramework)" />
  </ItemGroup>
</Project>

The downside to this approach: Each platform project's NuGet dependencies (at this time only the Windows project has one) isn't included properly. I can of course manually add a reference to the package in this new project, and there's probably also some MSBuild logic I can add to add those transitive package references as a direct one instead.

However, for now I'm looking into another approach instead: Keep the platform-specific NuGet packages and instead pack them all into a new "meta package" instead.

The bigger missing point for either approach is to get the APIs wrapped in a way that does the correct call to the right assembly at runtime. Still looking into that but using Core (or a new project) as the entry point, making that a reference assembly and that then calling into a manually loaded platform-specific assembly should work.

@theolivenbaum
Copy link

We do something like this (multiplatform bindings with single nuget) with our RocksDB bindings, if you want to check it out: https://github.com/curiosity-ai/rocksdb-sharp

@JBildstein
Copy link
Owner

Thanks for the suggestion and detailed explanation and examples!

I have tried various things to get a single Nuget package but nothing really worked so far. The main reasons I decide against it in the end (other than not working) was

  • easier platform specific custom code
  • inversion of control: Core project doesn't need to know about any OS code
  • the problem that the Windows project has a different SDK target (Desktop) than the other projects

The metapackage sounds like the most promising way to me but doesn't that mean that each platform specific build also contains the DLLs of all platforms?

Either way, I'm open to solutions for a single Nuget package as long as separate projects are still possible (for those that need it).

@theolivenbaum thank you for the example, always good to see something that works (the docs are a bit sparse on that topic). It's a bit different though from what I'd need here. You include native libs while I need to include .Net libs that should get referenced.

@bddckr
Copy link
Contributor Author

bddckr commented Jul 30, 2020

It's a bit different though from what I'd need here. You include native libs while I need to include .Net libs that should get referenced.

I think that's what I'm slowly able to confirm works fine for non-native assemblies, too: We just need to put the dlls into runtimes/$(Rid)/lib/$(TargetFramework) rather than runtimes/$(Rid)/native. The reason as to why this works is explained in the linked NuGet docs (emphasis mine):

Please note, NuGet always picks these compile or runtime assets from one folder so if there are some compatible assets from /ref then /lib will be ignored to add compile-time assemblies. Similarly, if there are some compatible assets from /runtimes then also /lib will be ignored for runtime.

So once we provide our libs via the runtimes folder we don't actually need anything in the root lib as it will be ignored. That is, as long as we have one runtime folder for each supported runtime identifier at least, otherwise it would fall back to the root lib folder just for that runtime.

Note the first sentence of this quote: As soon as we have a single platform-specific assembly in its runtime folder we also need to ensure that same folder has all the other assemblies we need. Ultimately this bloats the NuGet up a bit as the Core dll needs to be copied into all runtime folders. I think that's fine - it's not much, it compresses perfectly and more importantly it only matters on the developer machine.

The metapackage sounds like the most promising way to me but doesn't that mean that each platform specific build also contains the DLLs of all platforms?

I just finished some tests and it looks like NuGet only includes the dlls for the runtime(s) you build for. This matches the behavior of native libs from native - the build output only has what it needs.

NuGet allows us to differentiate between the assemblies we want to reference at compile time, and which ones to reference at runtime:

These assemblies will only be available at runtime, so if you want to provide the corresponding compile time assembly as well then have AnyCPU assembly in /ref/{tfm} folder.

So what this is saying is that we should put the assemblies we want to compile against into ref, while we put all runtime ones into runtimes. And it's just additionally smart about publishing for a runtime and only includes what you need from that list of supported runtimes from our NuGet package.

ref is important here: We want the developer to work on any OS, so we need to ensure the code they compile against is platform-agnostic. That is already true for the Core project, so we can also include that in ref. For this reason we should not include the OS-specific dlls in ref, though.
Normally you'd make the Core project output a reference assembly, but in our case Core is already agnostic, so it's perfectly fine to put Core into ref directly.

A good summary of all of this can be found here.


I just found a pretty good example of this solution! Microsoft.Data.SqlClient does what we want:

  1. Build for each OS manually first.
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/build.proj#L80-L84
    The project makes sure to only compile the OS-specific sources based on the OS you're building for. For our case we can instead just build the existing OS-specific projects in this step.
  2. Run a target to pack the build output from the previous step with a nuspec advising what to do.
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/build.proj#L50
    We can just define the details of the pack operation via another csproj instead of a nuspec I think, as my approach with <None> above from the other comments seems to work fine.
  3. The target literally just packs.
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/tools/targets/GenerateNugetPackage.targets#L10
    We can do this all while staying in a csproj.

The nuspec has a few interesting things to it.

  1. Exclude the Compile assets for any package it references that is not available on all OSs. At least I think that's the reason they exclude here. It might also just be a case of ensuring that the public API surface of this package does not add private assets only the package itself needs.
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/tools/specs/Microsoft.Data.SqlClient.nuspec#L59-L69
    Still no need for us to use a nuspec here - ExcludeAssets allows us to set this.
  2. Copy the reference assemblies into the right place (/ref).
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/tools/specs/Microsoft.Data.SqlClient.nuspec#L115-L118
    We can use <None> here just fine.
  3. Copy the runtime assemblies into the right place (/runtimes).
    https://github.com/dotnet/SqlClient/blob/7471403aac47ab724ae1885140f889c308ef0745/tools/specs/Microsoft.Data.SqlClient.nuspec#L166-L170
    Again, we can use <None> for this.

SpiderEye is a bit different when it comes to a few things: We might have an issue with this getting put in our auto-generated nuspec.

<frameworkReferences>
  <group targetFramework=".NETCoreApp3.0">
    <frameworkReference name="Microsoft.WindowsDesktop.App.WindowsForms" />
  </group>
</frameworkReferences>

This is from the SpiderEye.Windows project, because it uses <Project Sdk="Microsoft.NET.Sdk.WindowsDesktop"> and/or <UseWindowsForms>true</UseWindowsForms>. To handle this we can set <DisableTransitiveFrameworkReferences>true</DisableTransitiveFrameworkReferences> and leave it up to the user's project to do the following (see the docs):

<Project>
  <PropertyGroup>
    <TargetsWindows>false</TargetsWindows>
    <TargetsWindows Condition="'$(RuntimeIdentifier)' != '' AND $(RuntimeIdentifier.StartsWith('win'))">true</TargetsWindows>
  </PropertyGroup>

  <Import Condition="!$(TargetsWindows)" Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />
  <Import Condition="$(TargetsWindows)" Project="Sdk.props" Sdk="Microsoft.NET.Sdk.WindowsDesktop" />

  <!-- The actual project configuration. -->

  <Import Condition="!$(TargetsWindows)" Project="Sdk.targets" Sdk="Microsoft.NET.Sdk" />
  <Import Condition="$(TargetsWindows)" Project="Sdk.targets" Sdk="Microsoft.NET.Sdk.WindowsDesktop" />
</Project>

The Windows project also uses <PackageReference Include="Microsoft.Windows.SDK.Contracts" Version="10.0.18362.2005" PrivateAssets="all" /> but that doesn't propagate to the nuspec and consumers anyway due to PrivateAssets="all".


So with all of this knowledge this is the tl;dr of the solution I think:

  1. We leave all projects as is. We also continue offering the NuGets as is. This makes sure the reasons you listed are still supported and the user has the most flexibility, especially when they need that OS-specific custom logic.
  2. We add a new project, let's call it the SpiderEye.Unified project. This project references Core and the OS-specific projects and does all of the above steps via MSBuild configuration for us whenever we run its Pack target (which dotnet pack runs).
  3. To the new project we have to manually add any <PackageReference>s that are used in any of the referenced projects. Currently the only ones of interest are actually defined in Shared\SpiderEye.Shared.proj so we can just <Import> that file in the Unified project.
    Note: We can't use NuGet package references instead of project references here instead:
    • OS projects don't put their output into /runtimes currently.
    • Even if they would, they still would have to add the same to /ref to continue supporting direct referencing from users who want OS-specific projects on their end.
    • The /ref folder gets merged from all referenced packages, so we would be polluting the new project's output consumption with OS-specific assemblies.

The result is a new NuGet package that has Core in /ref and each OS-specific output in /runtimes (along with Core's output because NuGet only retrieves from one folder - the matching runtime folder).

Now the only missing piece is the need to define some API somewhere that users can call to initialize the Application in an OS-agnostic way. We already added a new project - Unified, so let's add it there 😄 This project's assembly thus also needs to be put in /ref and all runtime folders. We can use #ifdef runtime checks in this one to call the right API.

Additionally I want to look into making our own custom SDK to make the above csproj setup a user has to do in their project a single line. I.e. <Project Sdk="SpiderEye.Sdk" />.

I'm working on this over this weekend, we'll see if this works 😉

@JBildstein
Copy link
Owner

woah this is amazing 😲

I'm totally on board with SpiderEye.Unified and adding an OS-agnostic application init class in there sounds like a good way to do it.

To the new project we have to manually add any <PackageReference>s that are used in any of the referenced projects. Currently the only ones of interest are actually defined in Shared\SpiderEye.Shared.proj so we can just <Import> that file in the Unified project.

The windows project will soon need a reference to support the chromium edge webview (maybe macOS as well, I haven't looked into that yet). To avoid duplicated reference entries I think it'd be best to have a shared proj file for each platform that lists references and is then imported in both the actual OS project and the unified project. This way you can't forget to add references to both projects and version updates only have to be done once.

Having a custom SDK would be the icing on the cake. I didn't even know that it was possible to create a custom one, that's very cool.
If you manage to get that working I'd love to add the bit for copying client files into the assembly as well:

  <ItemGroup>
    <EmbeddedResource Include="Angular\dist\**">
      <LogicalName>%(RelativeDir)%(Filename)%(Extension)</LogicalName>
    </EmbeddedResource>
  </ItemGroup>

where the include path is configurable of course and maybe a flag to disable this as well.

Thank you so much for all that detective work, you got way farther than I did 😄
Keep me posted, this weekend I'll be working on some of the open issues and see how far I can get with chromium edge.

@bddckr
Copy link
Contributor Author

bddckr commented Aug 3, 2020

I ran into a few more issues, so it will be a bit until I can get this into a state that hopefully works. Then we can clean it up afterwards 😉

I'll update this issue once I can.

@JBildstein
Copy link
Owner

no worries, I didn't expect it to work without a fight 😄

If you like you can create a WIP pull request if you need any input or a second set of eyes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants