clockworklabs / SpacetimeDB

Multiplayer at the speed of light

https://spacetimedb.com

Other

4.12k stars 100 forks source link

Simplify C# source generator csproj #1143

Closed RReverser closed 2 weeks ago

RReverser commented 3 weeks ago

Description of Changes

Apparently custom scripts are no longer necessary, so removing to make maintenance simpler (noticed this while working on yet another source generator).

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

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!

[x] Built the NuGet package, referenced in a new project, checked that it still builds as expected.
[ ] Write a test you want a reviewer to do here, so they can check it off when they're satisfied.

RReverser commented 3 weeks ago

Testing instructions (this one is a bit tricky to test because it only affects built NuGet package and can't be tested with folder references):

Go into SpacetimeDB\crates\bindings-csharp\Codegen in your terminal.
Run dotnet pack to create a NuGet package, this will be built in bin\Release subfolder.
Go into bindings-csharp\Runtime and run the same dotnet pack.
Create a new C# module somewhere on your disk with spacetime init --lang csharp <folder-name>.
Open the generated StdbModule.csproj.

Add something like this (with your local paths) to <PropertyGroup>:

<RestoreSources>D:\SpacetimeDB\crates\bindings-csharp\Codegen\bin\Release;D:\SpacetimeDB\crates\bindings-csharp\Runtime\bin\Release</RestoreSources>

Build the C# module with dotnet build.

It should build without errors - if it does, the NuGet packages are still packed correctly after this PR.

kazimuth commented 3 weeks ago

I think it would make sense to add that as a smoketest, I can do that on top of this branch if needed.

RReverser commented 3 weeks ago

I think it would make sense to add that as a smoketest, I can do that on top of this branch if needed.

Sounds great if you could. Existing tests already cover any code changes, but yeah, we don't have anything in place for Nuget config changes specifically.

kazimuth commented 3 weeks ago

@RReverser done. I also made the smoketests run on windows, at least that one -- although you have to run them through Powershell, not Git Bash.

RReverser commented 2 weeks ago

@kazimuth Added couple of nits. Also - does this new test run automatically on CI or is it for manual run only for now?

kazimuth commented 2 weeks ago

It should run automatically because CI installs dotnet sdk 8.0.

The issue you raised is patched, if in a slightly silly way.

E: never mind, I am going to fix it the way you suggested instead because it is less silly.

kazimuth commented 2 weeks ago

Okay done

RReverser commented 2 weeks ago

It should run automatically because CI installs dotnet sdk 8.0.

Yeah I just wasn't sure if this test needs to be added explicitly to some list to run on CI. I checked the logs and see that it did run, so all good: https://github.com/clockworklabs/SpacetimeDB/actions/runs/8839614331/job/24273342965?pr=1143#step:5:1227

RReverser commented 2 weeks ago

Wanted to approve the PR, but of course I can't because I created it 😅 Between the two of us, I think it should be good now, so added to the merge queue.

RReverser commented 2 weeks ago

Thanks again for the test!