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

Overhaul the NuGet packaging infrastructure and the way the Core binaries get acquired. #146

Merged
merged 25 commits into from
Dec 1, 2022

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented Nov 17, 2022

When a user installs the TileDB.CSharp package from NuGet, they get the TileDB.CSharp.dll managed assembly, but also the native Core binaries for Windows, macOS and Linux on x64. This existing approach has many disadvantages:

  • The NuGet package's size is 18.13MB, which is considered big for a NuGet package.
    • Besides the size itself, the main problem is that users are forced to download the native binaries for all platforms, even if they don't need them.
    • This also means that adding support for a new platform will similarly increase the package size for everyone.
  • The C# interface version is tightly coupled to the Core version. If a new version of the Core gets released, C# users will have to wait for a new version of the C# interface to come, which can complicate the development process.

This PR fixes all of the above problems. The Core gets distributed via separate NuGet packages for each supported RID, called TileDB.Native.runtime.<rid>, and one metapackage that tells the .NET SDK which of the above to use, called TileDB.Native. The entire process is automated and all necessary packages for a published Core GitHub Release can be built with one command.

Advantages

The advantages are enormous:

  • The TileDB.CSharp package contains just the managed code and depends on TileDB.Native for the rest, significantly shrinking itself to just 54KB.
  • Acquiring the native binaries is pay-for-play; you will download only those that you need.
    • In fact if you are building a library that uses TileDB.CSharp you will download none!
    • This enables us to expose Apple Silicon binaries to C# users without worries, which I snuck in.
  • If a new Core (edit: patch) version releases, C# users will be able to use it immediately.
  • The repository's infrastructure got simplified, making onboarding significantly easier. There are no more CMake files, and contributors don't have to build the Core or copy native binaries.
  • The TileDB.CSharp package is also no more built with a .nuspec file; everything is controlled using regular project properties.
  • As part of adjusting the nightly builds to this new system, the nightly build workflow was rewritten to me more efficient, building the Core only the minimum required number of times (once per platform and per branch we test).

Drawbacks

Not necessarily drawbacks, but some things will change. They have been appropriately documented.

  • Users will need to explicitly opt into RID-specific builds. Just dotnet running a C# app that uses TileDB will fail with a DllNotFoundException. They will need to add a project property to their executable projects that enables .NET to download the RID-specific assets. See the UseCurrentRuntimeIdentifier property I added to the test and example projects.

    This experience is expected to improve in .NET 8. (Enable FDD + (implicit) RID-specific apps with RuntimeSpecific dotnet/sdk#26031)

  • Using custom Core builds got a little more complicated. The expected way to do it is to download the NuGet packages from a nightly build's artifacts and follow the instructions in Directory.Packages.props. If you want to test with your own Core build, it will be slightly more complicated, but there are instructions.

  • I cut down the tiledb-csharp pipeline, in particular the Stage-Release-Candidate, Test-NuGet-Release and Release jobs. The former two are not needed; the construction of the TileDB.CSharp NuGet package is handled entirely by the .NET SDK; there is no benefit in testing it ourselves, and the Release job will need to be rewritten into its own workflow, which I will do in a future PR.

Before merging this

To unblock myself from working on this, I temporarily uploaded the native packages to a personal MyGet feed of mine. Assuming we want these changes, we should upload the packages to the official nuget.org feed before merging this PR.

To create the packages checkout the PR's branch and run

dotnet pack .\scripts\nuget\GenerateNuGetPackages.proj -p:Version=2.12.2 -p:VersionTag=a9d60c8

It will produce some nupkg files to the scripts\nuget\packages folder; upload them to NuGet.

…ore.

For now it downloads the artifacts from GitHub Releases.
And add a property to them that seems to save time.
And disable fail-fast in the Run-Tests action matrix.
Now that we don't use a nuspec to pack the C# library there are little reasons to specifically test it with a packed NuGet package.
The Release job also went away; it would upload the 0.0.0-local package. We need a dedicated release workflow.
It does not apply to libraries.
I had originally removed the nuspec files from the native packages but changed my mind for aesthetic reasons since they generate an empty .NET Standard 2.0 dependency group.
They have SemVer 2.0 versions that identify the branch.
And support changing the native package under a different name in development builds.
…ackages.

The core is built in separate jobs and not once for each .NET version.
Make the version tag optional for development builds.
And fail if no native binaries were found in development builds.
@Shelnutt2
Copy link
Member

If a new Core version releases, C# users will be able to use it immediately.

Its important we always tie C# releases to specific core version. This is critical for support and understanding when a user is on C# x.y.z what core version that ties too. We keep a version mapping and compatibility for our APIs to help facilitate this. Its also important to tie to specific version for deprecation cycles.

@teo-tsirpanis
Copy link
Member Author

OK, we can easily make TileDB.CSharp demand an exact version of TileDB.Native.

@teo-tsirpanis
Copy link
Member Author

teo-tsirpanis commented Nov 24, 2022

I added a warning for those who try to build an RID-agnostic executable that depends on TileDB.CSharp. I manually tested that it appears.

Adding the warning to TileDB.Native would be more complicated; and we can assume that those who directly depend on it know what they are doing. I also simplified the README.

The .NET SDK is planning to enable RID-specific builds by default for those who target .NET 8 (dotnet/sdk#28412). NuGet/Home#10571 is also tracking improving the experience for cases when the RID is not specified.

Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really nice overall, 👍 to merge. But as a follow-up, I'd like to figure out how we can make the dev experience against a local core/libtiledb build more straightforward. For reference, with tiledb-py we can pass a --tiledb argument to setup.py, pointing to the core dist/ folder, and then I can just make changes and update the core build iteratively to test fixes (only rebuilding py bindings if I make a change on the python side). I'd like to aim for a similar workflow here in the future, if at all possible w/in the constraints of dotnet.

…iption.

This package is not intended to be directly referenced by the average developer, at least not without `TileDB.CSharp`.
@teo-tsirpanis
Copy link
Member Author

Great! Before I merge it we have to push the native packages to NuGet. Check the .nuspec files to see if all metadata are OK, and either build and upload them yourself or tell me to do it. 2.12.3 has missing Linux binaries so we will do 2.12.2.

@teo-tsirpanis teo-tsirpanis added the NO-MERGE Don't merge this PR yet. label Nov 24, 2022
@teo-tsirpanis
Copy link
Member Author

I'd like to figure out how we can make the dev experience against a local core/libtiledb build more straightforward.

There is room for improvement but until then, a trick that has helped me in the past is to replace the library in packages/Local.TileDB.Native.runtime.<rid>/0.0.0-local/runtimes/<rid>/native/ with a symlink to your development build. Once you are done you can delete the entire Local.TileDB.Native.runtime.<rid> folder to avoid accidentally using the local development build in the future.

@teo-tsirpanis
Copy link
Member Author

I removed the MyGet feed. CI is green and nightly builds succeed. Merging.

@teo-tsirpanis teo-tsirpanis merged commit 6f9fc19 into TileDB-Inc:main Dec 1, 2022
@teo-tsirpanis teo-tsirpanis deleted the core-nuget branch December 1, 2022 10:00
@teo-tsirpanis teo-tsirpanis removed the NO-MERGE Don't merge this PR yet. label Dec 1, 2022
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

Successfully merging this pull request may close these issues.

3 participants