Open ElDuderinos opened 1 year ago
This looks really interesting, I never expected anyone to add a whole new backend to Canopy. As I'm not familiar with C# at all, this may take a while to review, and it may happen that I decide I'm not able to take responsibility for maintaining this code, but I want to see if I can get things running.
The first issue I'm running into is that the tests don't run; I've installed the dotnet
Homebrew package (I'm on an M1 Mac) and running make test-cs
gives the following:
$ make test-cs
cd test/cs/choices && dotnet test --framework netcoreapp3.1
Determining projects to restore...
/Users/jcoglan/projects/jcoglan/canopy/test/cs/choices/choices_test.csproj : warning NU1701: Package 'MSTest.TestAdapter 2.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.1'. This package may not be fully compatible with your project.
All projects are up-to-date for restore.
/Users/jcoglan/projects/jcoglan/canopy/test/cs/choices/choices_test.csproj : warning NU1701: Package 'MSTest.TestAdapter 2.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.1'. This package may not be fully compatible with your project. [TargetFramework=netcoreapp3.1]
choices_test -> /Users/jcoglan/projects/jcoglan/canopy/test/cs/choices/bin/Debug/netcoreapp3.1/choices_test.dll
Test run for /Users/jcoglan/projects/jcoglan/canopy/test/cs/choices/bin/Debug/netcoreapp3.1/choices_test.dll (.NETCoreApp,Version=v3.1)
Microsoft (R) Test Execution Command Line Tool Version 17.4.0+c02ece877c62577810f893c44279ce79af820112 (arm64)
Copyright (c) Microsoft Corporation. All rights reserved.
Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
Could not find 'dotnet' host for the 'X64' architecture.
You can resolve the problem by installing the 'X64' .NET.
The specified framework can be found at:
- https://aka.ms/dotnet-download
Test Run Aborted.
make: *** [test-cs] Error 1
Can you help me with what I need to install or configure to get this running?
I would also be interested in knowing what the general experience of running C# on macOS is like. One major maintenance expense this project faces is running on many versions of many languages and we try to to reduce the impact of things by keeping things as simple of possible -- using a small subset of each language, not using any features that aren't really stable, not using any third party dependencies, and so on. Not being familiar with C#, it's impossible for me to assess this code in that way so I'd appreciate your opinion on how stable this code is likely to be and what issues I might run into.
M1 is officially supported only from .net 6, the Makefile requires you to set the framework otherwise it defaults to 3.1. See line 56: DOTNET_SDK?=netcoreapp3.1
To get the tests running on your M1 I think that running DOTNET_SDK=net6.0 make test-cs
will do the trick.
In regards to 3rd parties and keeping it simple, I used only native C# collections and no 3rd parties or external packages. The only fancy one I used is System.Linq in one of the unit tests, but it's anyway a pretty standard native C# library. The github actions tests .net core 2.1 through 6.0 which is quite wide, I also played around with it on framework 4.5 on Windows which is pretty ancient and it works smoothly.
Apologies, I have not had time to work on this over the summer but would be keen to get it integrated. The version of .NET on my machine now seems to be 7.0 (I guess due to a Homebrew update), and running the tests doesn't seem to have any effect:
$ DOTNET_SDK=net7.0 make test-cs
cd test/cs/choices && dotnet test --framework net7.0
Determining projects to restore...
/Users/jcoglan/projects/jcoglan/canopy/test/cs/choices/choices_test.csproj : warning NU1701: Package 'MSTest.TestAdapter 2.0.0' was restored using '.NETFramework,Version=v4.6.1, .NETFramework,Version=v4.6.2, .NETFramework,Version=v4.7, .NETFramework,Version=v4.7.1, .NETFramework,Version=v4.7.2, .NETFramework,Version=v4.8, .NETFramework,Version=v4.8.1' instead of the project target framework '.NETStandard,Version=v2.1'. This package may not be fully compatible with your project.
All projects are up-to-date for restore.
The build prints a series of messages like this, one for each test/cs
sub-project. Deliberately breaking the tests does not make this command emit any errors.
Could you fix the build so that it runs the tests, and preferably put all the tests under a single project so thr Makefile does not need to enumerate each test suite individually?
I'd also be obliged if you could rework the GitHub Actions config to use the matrix
system where possible.
Did you make meaningful changes to package.json
or just reformat it? If not, it would help a great deal if you could remove the part of the history that modifies this file as it leads to conflicts.
I have found that adding net7.0
to the <TargetFrameworks>
field in a csproj
file causes that project's tests to run, but you still get warnings about packages being the wrong versions. If possible, for future compatibility, I would like to arrange things so that those warnings do not happen and the project runs smoothly across all C# versions without noise that makes it harder to see the test output.
Trying to place multiple test suites in the same directory causes a problem of ParseHelper
and NodeWrapper
being defined multiple times. These should be defined once in a shared file and imported, rather than being copy-pasted between every test suite.
These should be defined once in a shared file and imported, rather than being copy-pasted between every test suite.
My apologies, on re-examining the Java tests, these helpers are defined once per suite because they need to make use of package-scoped definitions of TreeNode
, Label
and so on. A similar technique should be applied to the C# code so that all the tests can reside in the same project without namespace collisions. Being unfamiliar with C# I'm unsure how to achieve this.
A bit of experimentation indicates that I can get the tests working by wrapping each suite .cs
file in a namespace
block, matching the namespace names used in the Java code, and then adding all the necessary <Compile Include="..." />
directives to the .csproj
file, so that all the classes from each test/grammars/*
directory gets loaded. I'd like to know if there's an easier way of doing this where C# can figure out which packages to import based the using
directives in the code, without having to list out every file in the test suite in the XML config.
This exposes a further problem: for Java, each public class is required by the platform to be in its own source file. Does the same restriction apply to C#? If not I would like to reduce the number of files generated as much as possible to reduce the boilerplate that end users need to make use of the generated parsers.
Apologies for the flurry of comments yesterday, I believe I can resolve most of these issues myself. I'll make some changes on top of your work and then ask you to have a look at the final result to check it still meets your needs.
Add C# support to Canopy. Took most of the concept and also code from the Java support.