Restructure NuGet packaging #1440

RReverser · 2024-06-17T16:25:47Z

Since the 0.10 release and before this change we had 5 NuGet packages that need to be pushed on each release:

SpacetimeDB.BSATN.Codegen: Roslyn codegen for handling [SpacetimeDB.Type] attributes and generating [de]serialization code for marked types.
SpacetimeDB.BSATN.Runtime: Runtime API for [de]serialization.
SpacetimeDB.Runtime: Runtime APIs for interacting with SpacetimeDB host.
SpacetimeDB.Codegen: Roslyn codegen for handling [SpacetimeDB.Table] and [SpacetimeDB.Reducer] attributes and generating module definitions, reducer schedulers and table APIs on marked types and method. It also automatically included SpacetimeDB.BSATN.Codegen to generate ser/de APIs as well.
SpacetimeDB.ClientSDK: Client C# SDK.

C# users had to explicitly include:

Both SpacetimeDB.ClientSDK and SpacetimeDB.BSATN.Codegen as dependencies if they're developing a client.
Both SpacetimeDB.Runtime and SpacetimeDB.Codegen as dependencies if they're developing a module.

This PR restructures csproj configurations very differently, making both publishing and end-user consumption simpler. Now both *Codegen packages are internal, non-packable, dependencies and their DLLs are automatically included in *Runtime counterparts.

As maintainers, we only need to nuget push 3 packages now instead of 5:

SpacetimeDB.BSATN.Runtime - it will automatically include SpacetimeDB.BSATN.Codegen as part of the same package.
SpacetimeDB.Runtime - it will automatically include SpacetimeDB.Codegen in the same package.
SpacetimeDB.ClientSDK: Client C# SDK (it already has SpacetimeDB.BSATN.Runtime as a transitive dependency).

As a user, you'll need only 1 dependency now instead of 2:

SpacetimeDB.ClientSDK if you're developing a client.
SpacetimeDB.Runtime if you're developing a module.

Description of Changes

Please describe your change, mention any related tickets, and so on here.

API and ABI breaking changes

If this is an API or ABI breaking change, please apply the
corresponding GitHub label.

Expected complexity level and risk

1

How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.

This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.

If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.

Testing

Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!

Manually ran the updated csharp_module smoketest to verify that packaging still works as expected.

Since the 0.10 release and before this change we had 5 NuGet packages that need to be pushed on each release: - **SpacetimeDB.BSATN.Codegen**: Roslyn codegen for handling `[SpacetimeDB.Type]` attributes and generating [de]serialization code for marked types. - **SpacetimeDB.BSATN.Runtime**: Runtime API for [de]serialization. - **SpacetimeDB.Runtime**: Runtime APIs for interacting with SpacetimeDB host. - **SpacetimeDB.Codegen**: Roslyn codegen for handling `[SpacetimeDB.Table]` and `[SpacetimeDB.Reducer]` attributes and generating module definitions, reducer schedulers and table APIs on marked types and method. It also automatically included **SpacetimeDB.BSATN.Codegen** to generate ser/de APIs as well. - **SpacetimeDB.ClientSDK**: Client C# SDK. C# users had to explicitly include: - Both **SpacetimeDB.ClientSDK** and **SpacetimeDB.BSATN.Codegen** as dependencies if they're developing a client. - Both **SpacetimeDB.Runtime** and **SpacetimeDB.Codegen** as dependencies if they're developing a module. This PR restructures csproj configurations very differently, making both publishing and end-user consumption simpler. Now both `*Codegen` packages are internal, non-packable dependencies and their DLLs are automatically included in `*Runtime` counterparts. As maintainers, we only need to `nuget push` 3 packages now instead of 5: - **SpacetimeDB.BSATN.Runtime** - it will automatically include **SpacetimeDB.BSATN.Codegen** as part of the same package. - **SpacetimeDB.Runtime** - it will automatically include **SpacetimeDB.Codegen** in the same package. - **SpacetimeDB.ClientSDK**: Client C# SDK (it already has **SpacetimeDB.BSATN.Runtime** as a transitive dependency). As a user, you'll need only 1 dependency now instead of 2: - **SpacetimeDB.ClientSDK** if you're developing a client. - **SpacetimeDB.Runtime** if you're developing a module.

`dotnet build` doesn't do the final assembly of a single Wasm file, which might hide some errors.

bfops · 2024-06-17T21:17:30Z

Does this require a minor version bump? At first glance it seems like if we uploaded these as e.g. 0.10.1, then it would break users who are relying on 0.10.*? Because some of the packages would be updated and some wouldn't?

RReverser · 2024-06-17T21:20:14Z

Does this require a minor version bump? At first glance it seems like if we uploaded these as e.g. 0.10.1, then it would break users who are relying on 0.10.*? Because some of the packages would be updated and some wouldn't?

Correct, we need to leave this until 0.11 because existing users must remove the SpacetimeDB.{BSATN.}Codegen from their deps after this change. I should've added a label.

RReverser · 2024-06-17T21:20:53Z

I created a release-0.11 label and assigned it here.

bfops · 2024-06-17T21:24:31Z

smoketests/tests/csharp_module.py

@@ -41,7 +41,7 @@ def test_build_csharp_module(self):
                with open(csproj, "w") as f:
                    f.write(contents)

-                run_cmd("dotnet", "build", cwd=tmpdir, capture_stderr=True)
+                run_cmd("dotnet", "publish", cwd=tmpdir, capture_stderr=True)


is this related to the PR contents, or separate optimization?

It's related to testing this PR, see commit description. 79bb911

oh neat! I had no idea.

crates/bindings-csharp/BSATN.Runtime/BSATN.Runtime.csproj

bfops · 2024-06-17T21:27:09Z

crates/bindings-csharp/BSATN.Runtime/BSATN.Runtime.csproj

  </ItemGroup>

  <ItemGroup>
    <None Include="../logo.png" Pack="true" PackagePath="" />
    <None Include="../Runtime/README.md" Pack="true" PackagePath="" />
    <None Include="../LICENSE" Pack="true" PackagePath="" />
+    <!-- We want all users who depends on BSATN.Runtime to automatically get the Roslyn codegen component as well. -->
+    <None Include="../BSATN.Codegen/bin/$(Configuration)/netstandard2.0/SpacetimeDB.BSATN.Codegen.dll" Pack="true" PackagePath="analyzers/dotnet/cs" />


Dumb question - this says Pack="true" but the project above says <IsPackable>false</IsPackable> - I assume those don't actually conflict despite kinda sounding like they would?

another one - does this require any IncludeRuntimeDependency="false" like Codegen.csproj does below?

Dumb question - this says Pack="true" but the project above says <IsPackable>false</IsPackable> - I assume those don't actually conflict despite kinda sounding like they would?

Yeah the idea here is that BSATN.Codegen should not be packable (aka publishable) as a separate NuGet package after this change, but we explicitly pack the DLL it produces in the BSATN.Runtime package instead.

Perfect, thanks!

bfops · 2024-06-17T21:28:10Z

crates/bindings-csharp/Codegen/Codegen.csproj

  </ItemGroup>

  <ItemGroup>
-    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.5.0" PrivateAssets="all" />
+    <PackageReference Include="Microsoft.CodeAnalysis.CSharp" Version="4.3.1" PrivateAssets="all" />


why did this get downgraded?

For consistency with version used in BSATN.Codegen. It's not a strictly required downgrade, but that version already has the APIs we need and is available in Unity, so why not.

Cool, ty. Leaving this visible in case other/future reviewers have the same question.

bfops · 2024-06-17T21:31:35Z

crates/bindings-csharp/Codegen/Codegen.csproj

-    <ProjectReference Include="../BSATN.Codegen/BSATN.Codegen.csproj" PrivateAssets="all" />
+    <!-- This analyzer uses some shared helpers from the BSATN.Codegen one, so include it as a regular dependency -->
+    <!-- (*not* with OutputItemType=analyzer because we don't want to use that source generator on source of Codegen itself) -->
+    <ProjectReference Include="../BSATN.Codegen/BSATN.Codegen.csproj" />


sanity checking my understanding - we removed PrivateAssets="all" because we (now) do want the contents of BSATN.Codegen.csproj to be available to people who include Codegen.csproj, is that right?

It's just another one of "forth and back" change that doesn't matter in the end.

PrivateAssets="all" only matters for NuGet packaging and neither Codegen nor BSATN.Codegen are packable after this PR, so having or removing that attribute has zero difference.

I'd probably keep it removed just to avoid someone asking why we need PrivateAssets="all" on a seemingly no-op package in the future.

I removed PrivateAssets="all" on other codegen references as well for consistency & to reduce noise.

crates/bindings-csharp/BSATN.Codegen/BSATN.Codegen.csproj

crates/bindings-csharp/Codegen/Codegen.csproj

bfops · 2024-06-17T21:38:22Z

crates/bindings-csharp/Runtime/Runtime.csproj

@@ -41,6 +47,8 @@
    <None Include="build/" Pack="true" PackagePath="build" />
    <None Include="bindings.c" Pack="true" PackagePath="" />
    <None Include="driver.h" Pack="true" PackagePath="" />
+    <!-- We want all users who depends on Runtime to automatically get the Roslyn codegen component as well. -->
+    <None Include="../Codegen/bin/$(Configuration)/netstandard2.0/SpacetimeDB.Codegen.dll" Pack="true" PackagePath="analyzers/dotnet/cs" />


Again asking for my understanding, feel free to just say "trust me" if it's too much explaining - what's the difference between depending on the Codegen.csproj "directly" vs forcing it to build above and then depending on the dll?

Depending "directly" results in a Runtime nuget package with a dependency on a Codegen nuget package, so we'd still need to pack and publish both on each release.

However, including the DLL of the Codegen package directly in the Runtime one simply "merges" them together so we only need to publish a single package instead.

Co-authored-by: Ingvar Stepanyan <me@rreverser.com> Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>

crates/bindings-csharp/Runtime/Runtime.csproj

Co-authored-by: Zeke Foppa <196249+bfops@users.noreply.github.com> Signed-off-by: Ingvar Stepanyan <me@rreverser.com>

bfops

To the best of my ability to understand these packages, this seems good to me. If it ends up breaking something, maybe we can add more tests.

bfops · 2024-06-18T18:34:54Z

crates/bindings-csharp/Directory.Build.props

+    <None Include="$(MSBuildThisFileDirectory)/logo.png" Pack="true" PackagePath="" />
+    <None Include="$(MSBuildThisFileDirectory)/LICENSE" Pack="true" PackagePath="" />
+  </ItemGroup>
+</Project>


TIL this file! 👍

Those packages are already marked with <IsPackable>false</IsPackable> so this attribute only adds verbosity but has no extra effect.

Just making them more maintainable by splitting groups into more logical ones (general assembly metadata, build options, etc).

kurtismullins

This is awesome, thank you @RReverser!

I may need a hand to figure out which packages no longer need to be published when we do our releases (although it seems self-evident here) but overall, LGTM!

RReverser · 2024-06-18T19:29:56Z

I may need a hand to figure out which packages no longer need to be published when we do our releases (although it seems self-evident here)

I documented that in the 2nd part of the PR description, but let me know if you need anything else.

Signed-off-by: Ingvar Stepanyan <me@rreverser.com>

- Roslyn codegen won't need to be added as a separate dependency anymore (clockworklabs/SpacetimeDB#1440). - ByteArrayComparer, Identity, and Address will now live in BSATN.Runtime so that they're reused between the client and module SDKs.

bfops · 2024-07-01T21:43:36Z

These codegen changes broke SpacetimeDB master, because the updated NuGet packages aren't yet published. We probably should have done these things separately.

See discussion in #1471

## Description of Changes - Roslyn codegen won't need to be added as a separate dependency anymore. - ByteArrayComparer, Identity, and Address will now live in BSATN.Runtime so that they're reused between the client and module SDKs. ## API - [ ] This is an API breaking change to the SDK *If the API is breaking, please state below what will break* ## Requires SpacetimeDB PRs - [x] clockworklabs/SpacetimeDB#1440 - [ ] clockworklabs/SpacetimeDB#1455 --------- Co-authored-by: Zeke Foppa <github.com/bfops>

RReverser requested review from bfops and kurtismullins June 17, 2024 16:25

RReverser force-pushed the ingvar/csharp-revamp-packaging branch from 88643bf to 854738e Compare June 17, 2024 16:27

RReverser requested a review from kazimuth June 17, 2024 16:27

bfops added the release-any To be landed in any release window label Jun 17, 2024

RReverser added 2 commits June 17, 2024 18:18

Remove obsolete leftover

dfc858f

Use dotnet publish for C# smoketest

79bb911

`dotnet build` doesn't do the final assembly of a single Wasm file, which might hide some errors.

RReverser added release-0.11 and removed release-any To be landed in any release window labels Jun 17, 2024

bfops added the breaks library compatibility This change breaks the SpacetimeDB library interface label Jun 17, 2024

bfops reviewed Jun 17, 2024

View reviewed changes

bfops self-assigned this Jun 17, 2024

Update crates/bindings-csharp/BSATN.Runtime/BSATN.Runtime.csproj

81f45bb

Co-authored-by: Ingvar Stepanyan <me@rreverser.com> Signed-off-by: Zeke Foppa <196249+bfops@users.noreply.github.com>

bfops reviewed Jun 17, 2024

View reviewed changes

crates/bindings-csharp/Runtime/Runtime.csproj Outdated Show resolved Hide resolved

Apply suggestions from code review

c6428f6

Co-authored-by: Zeke Foppa <196249+bfops@users.noreply.github.com> Signed-off-by: Ingvar Stepanyan <me@rreverser.com>

bfops approved these changes Jun 18, 2024

View reviewed changes

bfops removed their assignment Jun 18, 2024

Extract common MSBuild properties

04acffc

bfops reviewed Jun 18, 2024

View reviewed changes

RReverser added 2 commits June 18, 2024 19:45

Remove PrivateAssets="all" on codegen

93d19e7

Those packages are already marked with <IsPackable>false</IsPackable> so this attribute only adds verbosity but has no extra effect.

Restructure property groups

51194ce

Just making them more maintainable by splitting groups into more logical ones (general assembly metadata, build options, etc).

kurtismullins approved these changes Jun 18, 2024

View reviewed changes

Merge branch 'master' into ingvar/csharp-revamp-packaging

09c5637

Signed-off-by: Ingvar Stepanyan <me@rreverser.com>

RReverser enabled auto-merge June 18, 2024 19:47

RReverser added this pull request to the merge queue Jun 18, 2024

Merged via the queue into master with commit 137f26d Jun 18, 2024
7 checks passed

RReverser deleted the ingvar/csharp-revamp-packaging branch June 18, 2024 23:31

bfops mentioned this pull request Jun 19, 2024

Bump version to 0.11.0 #1447

Closed

RReverser mentioned this pull request Jun 25, 2024

0.11 upgrades clockworklabs/com.clockworklabs.spacetimedbsdk#101

Merged

3 tasks

bfops mentioned this pull request Jul 1, 2024

Fix: Smoketests failing in master #1471

Closed

bfops mentioned this pull request Jul 2, 2024

Fix C# module smoketests #1475

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure NuGet packaging #1440

Restructure NuGet packaging #1440

RReverser commented Jun 17, 2024 •

edited

Loading

bfops commented Jun 17, 2024

RReverser commented Jun 17, 2024 •

edited

Loading

RReverser commented Jun 17, 2024

bfops Jun 17, 2024

RReverser Jun 17, 2024

bfops Jun 17, 2024

bfops Jun 17, 2024

bfops Jun 17, 2024

RReverser Jun 17, 2024

bfops Jun 17, 2024

bfops Jun 17, 2024

RReverser Jun 17, 2024

bfops Jun 17, 2024

bfops Jun 17, 2024

RReverser Jun 17, 2024 •

edited

Loading

RReverser Jun 18, 2024

bfops Jun 17, 2024

RReverser Jun 17, 2024

bfops left a comment

bfops Jun 18, 2024

kurtismullins left a comment

RReverser commented Jun 18, 2024

bfops commented Jul 1, 2024 •

edited

Loading

Restructure NuGet packaging #1440

Restructure NuGet packaging #1440

Conversation

RReverser commented Jun 17, 2024 • edited Loading

Description of Changes

API and ABI breaking changes

Expected complexity level and risk

Testing

bfops commented Jun 17, 2024

RReverser commented Jun 17, 2024 • edited Loading

RReverser commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RReverser Jun 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfops left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kurtismullins left a comment

Choose a reason for hiding this comment

RReverser commented Jun 18, 2024

bfops commented Jul 1, 2024 • edited Loading

RReverser commented Jun 17, 2024 •

edited

Loading

RReverser commented Jun 17, 2024 •

edited

Loading

RReverser Jun 17, 2024 •

edited

Loading

bfops commented Jul 1, 2024 •

edited

Loading