Skip to content

Conversation

@quackzar
Copy link
Contributor

@quackzar quackzar commented Aug 2, 2023

Change the rust subproject to be built more as a dotnet project with .rsproj. This is mostly motivated by a better attempt to specify the native runtimes to package for. Also the project directory have been renamed to Native.

Currently a fallback exists that also builds the version native to the system independent of what specified with --runtime. There probably still needs to be some work here to ensure the nuget package work well, and some considerations in how to bundle it i.e., should we package n different versions for different runtimes or a single version containing all n runtimes?
I would suggest the former being more reliable as downstream users can see if their architecture is supported or not.

Also implemented a crude benchmarking utility to check that we don't bottleneck concurrent invocations.

Lastly, changed from using net6.0 and net7.0 to only net7.0, there seems to some issues trying to target both, and the introduction of LibraryImport in .NET 7 greatly motivates this.

@quackzar quackzar requested a review from MartinSchmidt August 2, 2023 08:42
@MartinSchmidt
Copy link
Member

MartinSchmidt commented Aug 2, 2023

@Quacktiamauct I would opt for the latter, one nuget package, containing all platforms, otherwise any dependent application would have to add platform dependent build scripts yet again and add multiple packages. To ensure it as easy to use as possible, this nuget packages should solve this problem.

I am a bit unsure what you are trying to achieve with the restructuring?

As the PR is now, it seems like there is even more .csproj configuration, and not less, which doesn't make it simpler to read, at least not for me.

The current build system, builds the local platform when in debug, so it could be tested locally, and when building a release, it builds all the supported platforms, and adds the binaries to the required subfolders in the nuget package.

@quackzar
Copy link
Contributor Author

quackzar commented Aug 3, 2023

The goal was mostly experiments with making the build system more robust and decoupling the release/debug from a local/package, since it becomes hard to test the release version. This was mostly for easier testing/benchmarking, and sanity checking that it did include and use the correct runtimes.

But, apparently using RuntimeIdentifier to specify builds wasn't an effective method. So I have defaulted to just include the dynamic libraries if found/built. The .rsproj file have also been sort-of deprecated in favor of manually compiling, as that didn't work as great as I had hoped. The attempt here was trying to treat the rust project as a dependency in out itself, thus having it self-built when files changed etc.

The GitHub actions have also been updated to now instead directly compile the rust targets, however we should maybe extend these to cover other runtimes such as macOS (x86, arm) and Windows (arm).

@quackzar quackzar requested a review from MartinSchmidt August 4, 2023 07:39
@quackzar quackzar marked this pull request as ready for review August 7, 2023 07:40
@MartinSchmidt MartinSchmidt force-pushed the build-restructure branch 2 times, most recently from 03840c9 to ccf7d94 Compare August 21, 2023 11:00
Copy link
Member

@MartinSchmidt MartinSchmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Quacktiamauct I looked through it, all your changes looks good to me.
I made some changes to the workflow I though would make sense to include, will you look through it and comment back or merge it to main :)

@MartinSchmidt MartinSchmidt force-pushed the build-restructure branch 2 times, most recently from 52a709f to d18f74b Compare August 21, 2023 11:07
Added aditional targets (mac and windows)
Minor cleanup
Added CONTRIBUTING.md
@MartinSchmidt MartinSchmidt self-requested a review August 21, 2023 12:06
@quackzar
Copy link
Contributor Author

Looks great with the new workflows! Now the native container runtimes get to build the different libraries, so linking and such should no longer be an issue.

@MartinSchmidt
Copy link
Member

@Quacktiamauct you also need to review and accept it, otherwise I atleast wont be able to merge it to main :) but you can do it if you want to.

@quackzar
Copy link
Contributor Author

quackzar commented Aug 21, 2023

Apparently GitHub won't let do that since I created the PR in the first place. 😅

EDIT: It was dead-locked since you are the last to push something, so I have pushed a dummy commit.

@quackzar
Copy link
Contributor Author

So the new LibraryImport annotations are only a feature above net7.0

@quackzar quackzar requested a review from MartinSchmidt August 21, 2023 13:08
Copy link
Member

@MartinSchmidt MartinSchmidt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess you remove the 6.0 since the LibraryImport does not support it?

@quackzar
Copy link
Contributor Author

I guess you remove the 6.0 since the LibraryImport does not support it?

Exactly. It was introduced in 7.0 in favor of the old DllImport.

@quackzar quackzar merged commit c87fd60 into main Aug 21, 2023
@MartinSchmidt MartinSchmidt deleted the build-restructure branch August 21, 2023 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants