Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: dotnet targets #3791

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

chore: dotnet targets #3791

wants to merge 28 commits into from

Conversation

Seddryck
Copy link

@Seddryck Seddryck commented Nov 12, 2023

  • 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
Copy link
Author

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
Copy link
Author

Seddryck commented Nov 12, 2023

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
Copy link
Member

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

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

@vanillajonathan
Copy link
Collaborator

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
Copy link
Member

(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
Copy link
Author

Seddryck commented Nov 17, 2023

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
Copy link
Author

Seddryck commented Nov 17, 2023

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
Copy link
Author

Seddryck commented Nov 17, 2023

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
Copy link
Author

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
Copy link
Author

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
Copy link
Member

max-sixty commented Nov 17, 2023

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
Copy link
Collaborator

@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
Copy link
Author

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

@Seddryck Seddryck marked this pull request as ready for review November 19, 2023 14:50
@Seddryck
Copy link
Author

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

@max-sixty
Copy link
Member

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
Copy link
Member

eitsupi commented Dec 13, 2023

@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
Copy link
Member

max-sixty commented Dec 13, 2023

@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
Copy link
Member

eitsupi commented Feb 23, 2024

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
Copy link
Collaborator

vanillajonathan commented Feb 23, 2024

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
Copy link
Author

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

@max-sixty
Copy link
Member

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
Copy link
Author

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
Copy link
Collaborator

Can you add net8.0 too?

@Seddryck
Copy link
Author

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

@vanillajonathan
Copy link
Collaborator

Hey @Seddryck, hows it going?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants