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

Upgrade the .NET bindings to .NET Standard 2.0. #410

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JamesMenetrey
Copy link
Contributor

@JamesMenetrey JamesMenetrey commented Mar 19, 2019

  • Upgrade the .NET bindings to .NET Standard 2.0.
  • 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 (https://www.nuget.org/packages/keystoneengine.csharp/).

@71, @chaplin89 : could you please ack if you have some spare time please ?

Thanks and happy coding !
Cheers,
ZenLulz

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

aquynh commented Aug 2, 2019

please can anybody help to review this pull req?

Copy link
Contributor

@71 71 left a 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>
Copy link
Contributor

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()
Copy link
Contributor

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

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))
Copy link
Contributor

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.
Copy link
Contributor

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.

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.

3 participants