-
Notifications
You must be signed in to change notification settings - Fork 33
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
Introduce packaging, distribution and documentation for the C# driver #623
Conversation
PR Review ChecklistDo not edit the content of this comment. The PR reviewer should simply update this comment by ticking each review item below, as they get completed. Trivial Change
Code
Architecture
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't fully work on CircleCI, but it has some successful passes for some platforms (the whole workflow), so you can have a look. I'll put another comment once it's in the final state.
@@ -25,5 +25,5 @@ SET DEPLOY_ARTIFACT_USERNAME=%REPO_TYPEDB_USERNAME% | |||
SET DEPLOY_ARTIFACT_PASSWORD=%REPO_TYPEDB_PASSWORD% | |||
|
|||
SET /p VER=<VERSION | |||
bazel --output_user_root=C:\bazel run --verbose_failures --define version=%VER% //c:deploy-windows-x86_64-zip --compilation_mode=opt -- release | |||
bazel --output_user_root=C:\b run --verbose_failures --define version=%VER% //c:deploy-windows-x86_64-zip --compilation_mode=opt -- release |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lack symbols for Windows path. It started failing even within the Rust build steps, so we can't even change these names. Right now it has just several free symbols until 260...
|
||
ECHO Building and deploying windows package... | ||
SET DEPLOY_NUGET_API_KEY=%REPO_TYPEDB_PASSWORD% | ||
@REM TODO: Add new key for nuget as a new variable! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only for release, we'll configure it with @flyingsilverfin once the nuget bot account with the key is created.
@@ -97,4 +97,3 @@ Cargo.toml | |||
|
|||
# Temporary paket files for C# | |||
paket-files/ | |||
paket.lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we do normally commit the lock files for various languages' package dependencies!
@@ -31,13 +31,21 @@ csharp_library( | |||
"//csharp:typedb_driver_pinvoke", | |||
"@paket.csharp_deps//newtonsoft.json", | |||
], | |||
out = "TypeDB.Driver.Api", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's how I name the output dlls! All following their namespaces.
@@ -335,7 +335,7 @@ IThingType IConcept.AsThingType() | |||
public class Annotation : NativeObjectWrapper<Pinvoke.Annotation> | |||
{ | |||
/** | |||
* @hidden | |||
* @private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The equivalent of @hidden for the java documentation. It causes warnings that a member lacks documentation while the doxygen build (which is dumb), but I find it okay.
) | ||
|
||
|
||
swig_native_nuget_pack( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Produces a set of native libs, picks one for our current platform to push further.
) | ||
|
||
|
||
nuget_push( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushes the generated .nuspec
and .snuspec
files based on the snapshot / release
cmd flag.
nuget_pack( | ||
name = "driver-csharp-pack", | ||
id = "TypeDB.Driver", | ||
libs = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We include all the libs, so their namespaces are available for users. However, only Api
and Common
are documented, so we consider it as something okay.
) | ||
|
||
|
||
# TODO: Not hermetic - requires doxygen on the host |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inherit this TODO from C++, not touching it now.
@@ -25,7 +25,7 @@ | |||
|
|||
namespace TypeDB.Driver | |||
{ | |||
public static class TypeDB | |||
public static class Drivers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not know how to call this entry class, and TypeDB
was okay while I was using libs as Bazel dependencies. But once I started calling TypeDB.CoreDriver
in the outer CS project, it started conflicting with the namespace name (TypeDB.Driver
), which was expected... to fire earlier.
I have no idea how to call it, so it is Drivers
. Feel free to suggest better options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'll be ok for now!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Connection
tests are now explicit Examples
.
} | ||
|
||
[Test] | ||
public void DocExample() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is for copypasting into README. The upper ones show the same workflow with different ways of doing things.
# name = "vaticle_dependencies", | ||
# remote = "https://github.com/vaticle/dependencies", | ||
# commit = "7518f8adaf5714512a8af96128b15cca7dedd475", # sync-marker: do not remove this comment, this is used for sync-dependencies by @vaticle_dependencies | ||
# ) # TODO: return afther the PR is merged! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix it once the other pull requests are merged.
@@ -32,7 +32,7 @@ class AsciiDocTableBuilder(private val headers: List<String>) { | |||
} | |||
|
|||
private fun header(): String { | |||
return "[cols=\"~" + ",~".repeat(this.headers.size - 1) + | |||
return "[cols=\"" + ",".repeat(this.headers.size - 1) + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vladimir asked me to remove these symbols as we can't understand what they do and they just distract us while debugging tables' behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
val argsMap = argsList.toMap() | ||
val argsMap = argsList.associate { (first, second) -> | ||
Pair( | ||
addZeroWidthWhitespaces(first), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Zero-width whitespaces help tables to squeeze gently and look fresh. Needed for special cases when we store Some.Namespaces.Inside.The.Tables.Without.Spaces.Which.Freaks.Browsers.Out.
7c60c10
to
035c7c3
Compare
Usage and product changes
We introduce packaging, distribution and documentation C# driver for TypeDB (the original driver PR). It is built using the cross-platform .NET 6 framework.
Usage:
The driver is distributed as a series of Nuget packages. To use the driver, import the latest versions of the driver (TypeDB.Driver) and its Pinvoke runtime (TypeDB.Driver.Pinvoke) suitable for your platform.
CS project:
Here is an example from a
.csproj
for MacOS x86-64:If you aim to build a platform-independent package, reference all the needed runtimes (it will affect the size of your application by downloading a respective set of platform-specific dynamic libraries):
Bazel:
TypeDB.Driver
andTypeDB.Driver.Pinvoke
nuget packages as dependencies to your build rule.//csharp:driver-csharp
(the driver itself),//csharp/Api:api
(exposed Api namespace),//csharp/Common:common
(exposed Common namespace) as dependencies.A simple usage example (see
csharp/Test/Integration/Examples
for more):Implementation
Documentation:
Documentation for C# is generated in a similar way as the C/C++ ones:
doxygen
->adoc
. This way, the C# parser has some extra logic of parsing the intermediate doxygen files but follows a similar (copypasted) structure. Additionally, some small bugs for both old parsers have been fixed.Distribution:
We decided to distribute the C# driver as a series of packages to allow the end user to choose between multi-platform and compactness. This way, we have:
TypeDB.Driver.{X-X-X-version}.nupkg
- the main package with the driver, which fails in runtime if no runtime is additionally provided.TypeDB.Driver.Pinvoke.{X-X-X-version}.{platform}.nupkg
- platform-specific runtimes (osx-x64
,osx-arm64
,linux-x64
,linux-arm64
,win-x64
).The distribution is implemented via several bazel rules:
nuget_pack
(with a wrapper for native packages in this PR) andnuget_push
and their respective calls for both runtime and main packages.Tests:
Integration tests use NUnit framework as a built-in framework for C# bazel test rules. This PR presents both
test-core
andtest-cloud
as small usage examples within the integration tests. These tests are also triggered by Factory.There is also a new Deployment test, which creates a
.csproj
project and tries to fetch all the needed (and all the available runtime) packages to use them for another small C# program. This test is triggered by CircleCI.