search

clockworklabs / com.clockworklabs.spacetimedbsdk

The SpacetimeDB SDK for C# clients
https://spacetimedb.com
Apache License 2.0
6 stars 0 forks source link

Merge Unity SDK into C# SDK #117

Closed RReverser closed 2 weeks ago

RReverser commented 1 month ago

Description of Changes

This creates a frankenstein monster of a repo that is compatible with both .NET / MSBuild as well as can be consumed as a Unity package.

After this change, this repo should be compatible with both .NET CLI (e.g. dotnet test) as well as consumable from Unity, but due to amount of metadata changes, it needs further testing before we merge repos properly.

Fixes #114.

API

If the API is breaking, please state below what will break

Requires SpacetimeDB PRs

List any PRs here that are required for this SDK change to work

bfops commented 3 weeks ago

One thing to note is that if we make this the new com.clockworklabs.spacetimedbsdk repo, we'll lose all the branches/tags from the other repo. I'm not sure which ones bitcraft (or other users) are still using.

bfops commented 3 weeks ago

Unity gives me this error when I use this merged branch:

SpacetimeDB.BSATN.Runtime references netstandard, Version=2.1.0.0, Culture=neutral, PublicKeyToken=cc7b13ffcd2ddd51. A roslyn analyzer should reference netstandard version 2.0

It doesn't seem to prevent it from running, but I don't know if it'll cause any issues down the line.

bfops commented 3 weeks ago

Hm I get these errors when I run dotnet test now:

[11:00:51] ~/work/clockwork/spacetimedb-csharp-sdk/tests~
$ dotnet test
  Determining projects to restore...
  Restored /mnt/nutera/work/spacetimedb-csharp-sdk/tests~/tests.csproj (in 362 ms).
  1 of 2 projects are up-to-date for restore.
/mnt/nutera/work/spacetimedb-csharp-sdk/tests/obj/Debug/net8.0/tests.GlobalUsings.g.cs(2,1): error CS8773: Feature 'global using directive' is not available in C# 9.0. Please use language version 10.0 or greater. [/mnt/nutera/work/spacetimedb-csharp-sdk/SpacetimeDB.ClientSDK.csproj]
/mnt/nutera/work/spacetimedb-csharp-sdk/tests/obj/Debug/net8.0/tests.GlobalUsings.g.cs(3,1): error CS8773: Feature 'global using directive' is not available in C# 9.0. Please use language version 10.0 or greater. [/mnt/nutera/work/spacetimedb-csharp-sdk/SpacetimeDB.ClientSDK.csproj]
/mnt/nutera/work/spacetimedb-csharp-sdk/tests/obj/Debug/net8.0/tests.GlobalUsings.g.cs(4,1): error CS8773: Feature 'global using directive' is not available in C# 9.0. Please use language version 10.0 or greater. [/mnt/nutera/work/spacetimedb-csharp-sdk/SpacetimeDB.ClientSDK.csproj]
...
...
RReverser commented 3 weeks ago

Hm I get these errors when I run dotnet test now:

Weird, it works on CI.

Can you try to clean all the bin/obj folders and retry?

RReverser commented 3 weeks ago

Unity gives me this error when I use this merged branch:

That seems really odd as that DLL is not marked as RoslynAnalyzer :/ I didn't see this issue.

bfops commented 3 weeks ago

Hm I get these errors when I run dotnet test now:

Weird, it works on CI.

Can you try to clean all the bin/obj folders and retry?

Works now! I thought I ran git clean -fdx on everything but I must have missed something :shrug:

bfops commented 3 weeks ago

Unity gives me this error when I use this merged branch:

That seems really odd as that DLL is not marked as RoslynAnalyzer :/ I didn't see this issue.

Wouldn't it be because it bundles in SpacetimeDB.BSATN.Codegen now?

RReverser commented 3 weeks ago

Wouldn't it be because it bundles in SpacetimeDB.BSATN.Codegen now?

Shouldn't be because only Codegen.dll.meta has RoslynAnalyzer label: https://github.com/clockworklabs/spacetimedb-csharp-sdk/blob/0d135c8abe579a1a3e83927c4ee79329baafcf6f/packages/spacetimedb.bsatn.runtime/0.11.0/analyzers/dotnet/cs/SpacetimeDB.BSATN.Codegen.dll.meta#L4

So it's odd that it complains about Runtime.dll.meta which is not a source generator.

RReverser commented 3 weeks ago

It doesn't seem to prevent it from running, but I don't know if it'll cause any issues down the line.

Just in case, did this error also disappear by any chance after git clean?

bfops commented 3 weeks ago

It doesn't seem to prevent it from running, but I don't know if it'll cause any issues down the line.

Just in case, did this error also disappear by any chance after git clean?

The error only came up on first import, not on subsequent loads.

RReverser commented 3 weeks ago

I still don't know the best way to deal with branch names/tags after the repo rename though (#117 (comment))

Good point, I don't know either. Let's bring it up in the meeting.

bfops commented 3 weeks ago

It doesn't seem to prevent it from running, but I don't know if it'll cause any issues down the line.

Just in case, did this error also disappear by any chance after git clean?

Actually I think this must be unrelated to a git clean since this is fetched directly from the remote branch in BitCraftClient.

RReverser commented 3 weeks ago

Actually I think this must be unrelated to a git clean since this is fetched directly from the remote branch in BitCraftClient.

But you said you don't see it anymore, right?

bfops commented 3 weeks ago

Actually I think this must be unrelated to a git clean since this is fetched directly from the remote branch in BitCraftClient.

But you said you don't see it anymore, right?

I see it consistently when first loading the package (i.e. when switching Packages/manifest.json to point at this branch). I don't see it on subsequent reopens of Unity. It reappears if I switch to a different version of the package and then switch back.

RReverser commented 2 weeks ago

I see it consistently when first loading the package (i.e. when switching Packages/manifest.json to point at this branch). I don't see it on subsequent reopens of Unity. It reappears if I switch to a different version of the package and then switch back.

I thought maybe GUID in meta files got clashes between previous / modern versions of DLLs, but doesn't seem so. Idk, I'm out of ideas, but if it only happens when switching between package versions before / after the change, it sounds like some internal Unity cache and hopefully won't affect end users.

bfops commented 2 weeks ago

(just to follow up here - I should not have mindlessly merged master into this branch without thinking about the implications. After this merged, we had to do #123 in order to revert back to the state in this PR.)