-
Notifications
You must be signed in to change notification settings - Fork 456
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
Upgrade the .NET bindings to .NET Standard 2.0. #410
base: master
Are you sure you want to change the base?
Conversation
Modify how the bindings locate the native DLL, so it depends on the architecture of the running process. Create a NuGet package with Keystone 0.9.1 and upload it on nuget.org.
please can anybody help to review this pull req? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I was away at the time and completely forgot.
|
||
<PackageId>Keystone.Net</PackageId> | ||
<PackageId>keystoneengine.csharp</PackageId> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why renaming to KeystoneEngine.CSharp
? Usually packages that end with .CSharp
are related to the language itself (ie. Microsoft.CodeAnalysis.CSharp
). Bindings are available to all of .NET (not only C#) and thus usually end with .NET.
However this does pose a problem: How do we decrease the current version? (I'm not sure NuGet allows this).
/// <remarks> | ||
/// Taken from: http://stackoverflow.com/questions/10852634/using-a-32bit-or-64bit-dll-in-c-sharp-dllimport | ||
/// </remarks> | ||
static NativeInterop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda curious about this: Is it ever needed? I removed it from the original commit because after tests in different environments I never ended up needing this.
@@ -1,28 +1,36 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> | |||
|
|||
<PropertyGroup> | |||
<TargetFramework>netstandard1.1</TargetFramework> | |||
<TargetFramework>netstandard2.0</TargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're not using features only available in .NET Standard 2.0 (AFAIK), I think we should try targeting multiple frameworks by replacing
<TargetFramework>netstandard2.0</TargetFramework>
by
<TargetFrameworks>netstandard1.1;netstandard2.0</TargetFrameworks>
## Contributors | ||
Authors: | ||
|
||
- Grégoire Geis ([https://github.com/71](@71)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Link and text are swapped.
Ping the contributors of the .NET bindings when submitting a pull request, so your changes can be peer reviewed. | ||
|
||
#### NuGet package update | ||
Once your pull request has been accepted, please contact [@ZenLulz](https://github.com/ZenLulz/) so either he updates the library for you or add you to the project as a contributor on nuget.org. An example of *nupkg* is provided in this folder, as it requires to have a very specific configuration, because it embeds unmanaged libraries. The picture *nuget-package-config.png* details the structure and content of a NuGet package, ready to be deployed. Please reuse and test the package before pushing it onto nuget.org, as there is no possible roll back. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we end up keeping the existing package name, let's remember to modify this line.
@71, @chaplin89 : could you please ack if you have some spare time please ?
Thanks and happy coding !
Cheers,
ZenLulz