chore: dotnet targets

[Seddryck]

• Add .editorconfig and improve .gitignore files to ensure more consistency
• Move package definition and shared properties between the two projects in .props and .targets to ensure long-term consistency
• Switch from Net Standard 2.0 to net6.0 and net7.0 to be able to use null tracking (huge step forward for maintenance)
• Review the package structure to include the native dlls (Don't need anymore to manually copy them to the bin folder for consumer of the package) [Windows only]
• Update Readme.md related to installation instructions
• Update function description in the code to fix some mistakes
• Refactor exception messages
• Fix the name of the dll to import from libprqlc.dll to prqlc.dll and explicitly set ASCII encoding
• Update Readme.md related to how to use it (was already outdated)
• Make PrqlCompilerOptions immutable
• Set github-actions for build/tests/package
• Create/Define NuGet repository
• Set github-actions for publish

[Seddryck]

Checked with:

dotnet build prql-net.sln -p:version="0.10.1" -c Release /p:ContinuousIntegrationBuild=true --nologo
dotnet test PrqlCompiler.tests\PrqlCompiler.Tests.csproj -c release --runtime win-x64  --nologo
dotnet pack PrqlCompiler -p:version="0.10.1" -c Release --include-symbols --no-build --nologo

[Seddryck]

Integration of the package (built with commands above and stored into a local feed) into an external project is successful: https://github.com/Seddryck/DubUrl/tree/feature/prql

[max-sixty]

@Seddryck this looks great! Thanks for making so much progress.

If we can add a GHA, we can definitely merge...

[vanillajonathan]

Where did you get the .editorconfig from? Is this your own personal preferences or some common convention?

Someone could argue that the .gitconfig is a bit excessive (since it has ignores for stuff that we don't use), but that is okay. I realize it is from some template somewhere.

Not sure there is much benefit to using Directory.Build.props and Directory.Build.targets here since we only have two projects and I've mostly seen these in huge monorepos. That's okay though, its probably just me who haven't seen the light yet. 😂️

I intentionally targeted netstandard2.0 in order to get compatibility with both .NET and .NET Framework. Targeting net6.0 and net7.0 we lose compatibility with the old .NET Framework. I am not really opposed to that though, because I really like modern .NET with nullable reference types. NET 8.0 just got released though, so you should target that.
If we're dropping .NET Framework then maybe it would be time to start considering the LibraryImport attribute which was introduced in .NET 7 to replace the DllImport attribute
If we drop netstandard2.0 you should use file-scoped namespaces too.

Now that you load the DLL from ..\..\..\..\target\release\prqlc.dll I guess it can be tricky to use this in a real world project where you wouldn't have your project in that directory. The README says it will look in /bin/Debug/net7.0/. That seems contradictory.

I don't know why you explicitly set ANSI coding, maybe you can enlighten me?
Does this do anything for us?

We do have a GitHub Action at:
https://github.com/PRQL/prql/blob/main/.github/workflows/test-dotnet.yaml

We have some problems which I have not been able to figure out, if you're good at FFI in .NET maybe you could take a look?
When there is an error, we get wrong values in the Start and Stop properties in the Span, as well as the StartLine, StartCol, EndLine and EndCol properties in the SourceLocation.

[max-sixty]

(just to clarify, while I appreciate the comments & questions from @vanillajonathan , the main thing we'd want in order to merge is to integrate with our GHA, so future changes can't break the code. Other discussions are very welcome, but not a gate for merging. Thanks!)

[Seddryck]

The .editorconfig is a classical one is there anything that is shocking?

The .gitignore is also a classical one but it has been extended with some stuffs that could be weird for this project, I confess. In fact, I added these two files because I add some issues with the light .gitignore that was on the repo. But feel free to suggest other versions. I'm not married with them.

Regarding the drop of .netstandard, it's related to nullable types. Not sure that many people have a project targeting .NET Framework and depending on a package still not published on Nuget 🧐 It's a balance between maintainability and legacy backward compatibility.

Yep, I'll add .NET 8.0 on my next edit. TBH was waiting official release to prevent use of RC SDK.

File scoped namespace, indeed better to change it now that when you have 200 files.

The main benefit of .props and .targets is to have more files leading to smaller .csproj files with better separation of concerns. Your mileage may vary.

[Seddryck]

Regarding DllImport or LibraryImport, I was planning to switch when also targeting Linux and MacOS.

About ANSI, you could just try with Unicode to see what's happening, the tests were failing on my side. I didn't investigate further but at least now, it's visible and people who don't like it can take a look by themselves.

[Seddryck]

I'm not following you regarding the path from which the prql library is loaded.

I think I'm just using the relative path to include the dll in the package. The runtime import is not related to this path. As mentioned above, I used this package in another project without issues. So, except if I'm really lucky and I've the same structure in the other project, I have troubles to consider that I really have a relative path dependency when I load the library at runtime. Could you be more explicit where I made a mistake?

[Seddryck]

I'm not familiar with GitHub actions and workflows and took a bit of time to check GitHub docs around that. Should be ok soon if it's not too complex to deploy them.

[Seddryck]

Thx for the heads up on the hidden issues with FFI. I'll add test to check this after the work on the GHA.

I'm clearly not a specialist around FFI but who is?

[max-sixty]

FYI we changed some names to fix a name conflict on the rust side — so to minimize work for you, I merged main into your branch + resolved conflicts. The change on our side was that libprqlc.dll is now libprqlc_lib.dll. Hope that's OK, and it hasn't broken the "Fix the name of the dll to import from libprqlc.dll to prqlc.dll and explicitly set ASCII encoding" item...

[vanillajonathan]

@Seddryck

The .editorconfig is a classical one is there anything that is shocking?

I haven't looked much into it so I don't know if there is anything shocking in there. I was wondering if it was based upon your or someones personal preferences, because I think we should follow upstreams .NET conventions.

About ANSI, you could just try with Unicode to see what's happening, the tests were failing on my side. I didn't investigate further but at least now, it's visible and people who don't like it can take a look by themselves.

I think CharSet.Ansi is not needed, because the documentation seems to indicate that it is the default anyway.

I'm not following you regarding the path from which the prql library is loaded.

I think I'm just using the relative path to include the dll in the package. The runtime import is not related to this path. As mentioned above, I used this package in another project without issues. So, except if I'm really lucky and I've the same structure in the other project, I have troubles to consider that I really have a relative path dependency when I load the library at runtime. Could you be more explicit where I made a mistake?

Maybe it is not a problem then, I just saw:

<None Include="..\..\..\..\target\release\libprqlc_lib.dll">

and was thinking maybe that could somehow be a problem, but now that you say it, yeah I think you're right, it's not where it's loaded from at runtime.

I just load it a runtime from the build directory because it was easy but I don't know if that is the way to go, because I don't know what the best practices are for when using FFI libraries. Maybe it is to embed it, I don't know.

Thx for the heads up on the hidden issues with FFI. I'll add test to check this after the work on the GHA.

I'm clearly not a specialist around FFI but who is?

Yeah, this FFI is really tricky.

[Seddryck]

@max-sixty , I'm checking that today and also the GHA.

[Seddryck]

I dropped a message on Discord (channel #bindings) regarding the upload to NuGet.org

[max-sixty]

I dropped a message on Discord (channel #bindings) regarding the upload to NuGet.org

Awesome, thanks for getting the GHA working!

I just set up a NuGet account and set up a Gravatar profile for [email protected]. Shall I add you to the NuGet account so you can test it?

[eitsupi]

@max-sixty There was a bit of discussion on Discord. I think we need to consider whether to merge this and continue with mono repo, or split the repository by language.

[max-sixty]

@max-sixty There was a bit of discussion on Discord. I think we need to consider whether to merge this and continue with mono repo, or split the repository by language.

I'll check out discord — sorry I've been off for a while.

I wrote up a quick schematic for whether when I think we should separate into a new repo, interested in feedback

Factor	Rationale	Example
Does someone want to sign up to maintain a repo?	A different repo is harder for the core team to maintain	tree-sitter-prql is well maintained
Can it change independently from the compiler?	If it's in a different repo, it can't be changed in lockstep with the compiler	prql-vscode is fine to change "behind" the lang
Would a separate repo invite new contributors?	A monorepo with all the rust code can be less inviting for those familiar with other langs	prql-vscode had some JS-only contributors
Is there an convention for a stand-alone repo?	A small number of ecosystems require a separate repo	homebrew-prql needs to be named that way for a Homebrew tap

[eitsupi]

I believe the library binaries are now attached to the GitHub release. What do you want to do with this PR?

I think monorepo is too much of a maintenance burden and would prefer move into another repo.

[vanillajonathan]

shields.io have badges for package version and package downloads for NuGet packages.
https://shields.io/badges/nu-get-version
https://shields.io/badges/nu-get-downloads

[Seddryck]

I do believe that the easiest is to move forward with a specific repo dedicated to these bindings.

[max-sixty]

Yes, sorry if I was unclear — if @Seddryck is up for maintaining it, I'd very much welcome another repo containing the dotnet bindings. It would be a great contribution to the PRQL project. (And FWIW I don't think it would require much maintenance...)

What would be next to make progress?

[Seddryck]

I won't be able to work on this next 10 days but will take it back after.

I agree that I'm not expecting a lot of maintenance.

[vanillajonathan]

Can you add net8.0 too?

[Seddryck]

Yes surely, I'll work on this during the next days.

[vanillajonathan]

Hey @Seddryck, hows it going?

[vanillajonathan]

Hey @Seddryck, hows it going?