-
Notifications
You must be signed in to change notification settings - Fork 267
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
.NET bindings #73
.NET bindings #73
Conversation
I hope this PR will soon be reviewed. |
Please don't add just-bugging comments like that. I just returned from a long trip and am overwhelmed a bit. And this is a big patch to review and has implications I have to deal with (such as manually updating a generated makefile, which is bad but OTOH you had to do it because the tool used doesn't support C#...). Just two nitpicky notes a glance, before digging into it further (which I'll try to do a.s.a.p.):
|
The rest is doable :) |
Neither does the WinSparkle API. It's immediately obvious to anyone reading
WinSparkle does the normal and sensible thing that everybody does: using the shortest code that does the job, i.e. -2 or -3. Not sure how that's related to bindings, though, as far as I can tell it should be pretty irrelevant... |
@@ -1,9 +1,44 @@ | |||
# User-specific files |
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.
This seems overreaching, I think you must have pulled it from some presets repo. Let’s keep the ignores to what the projects actually produce?
No more comments from me, sorry for the delay — and thanks! The API is a bit low-level, basically 1-to-1 mapping of the C one. That’s certainly good as the first step, but a proper native C# API would be better (e.g. things could be done with properties, there’s some inconsistency here between what is and what isn’t “propertized”). Some way to get metadata from the main assembly would be nice too, because the native-C version can do that (with native metadata). Honestly, I’m not sure how .NET works in this regard, maybe the compiled assemblies have |
Any change of having at least some of the issues fixed? Would be a shame if nothing came out of it after you spent time implementing this PR and I spent time taking it seriously and reviewing it... |
I'm working on it! Just that University takes a lot of time at the end of the year .. |
Oops, sorry — no problem at all! I was just curious about the status, thanks for letting me know. Good luck with your studies! |
dad153a
to
acc00d8
Compare
Big update:
For the moment, metadata are extracted from the (managed structures of) main assembly. PS: thanks :) |
get { return _version; } | ||
set | ||
{ | ||
Debug.Assert(!_initialized, "Can't set Version after initialization !"); |
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.
initialization !
-> initialization!
.
Including a space before punctuation is not proper.
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 French touch who go out !
Thanks for contributing! Unfortunately there were the following style issues with your Pull Request:
Please see https://github.com/agis-/git-style-guide (you can use This message was auto-generated by https://gitcop.com |
c27d034
to
434fceb
Compare
); | ||
|
||
[UnmanagedFunctionPointer(CallingConvention.StdCall)] | ||
public delegate bool CanShutdownCallback(); |
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.
Is that correct? The corresponding C declaration is as follows:
typedef int (__cdecl *win_sparkle_can_shutdown_callback_t)();
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.
It certainly isn’t, thanks for catching that! WinSparkle C API uses cdecl, wrapping it as stdcall will crash.
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.
The same applies to the other delegate declarations.
How about that bool
vs. int
return type. Will that be marshalled correctly?
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 will correct that asap and will verify the other declaration.
While trying to get this to compile I'm getting a "Missing WinSparkle.dll" in the example app. Any idea how I can get this to run? |
You can only add references to .NET assemblies or COM components. Native DLLs like WinSparkle.dll must be added to your project like you would add a content file (e.g., .xml, .html). The File Properties should be configured as follows:
|
You must manually copy the native DLL by hand to do the work, for the moment. I will add a post-build-script or make that native DLL is packed into the managed one in a near future. |
I was going to say that was IMHO the best approach, System.Data.SQLite used to do it, but apparently they don’t anymore? |
@Herve-M My solution does take the build type into account. I'm using that for many different content files, including DLLs. If the output path is This also works in multi-project solutions where project A references project B, for example, and project B defines a DLL as Content / Copy if newer. After building, you will find the DLL in project B's and A's output directories. And if you switch between the Debug and Release configurations, you will find it in both the |
@vslavik I'm using the Costura.Fody nuget package (also hosted as a project on GitHub) to embed WinSparkle.dll in a managed assembly. For example, in my solution I have a C# project into which that nuget package (and a prerequisite package called Fody) is installed. That project contains a folder called |
To clarify, this (and what I now realize probably is indeed what @Herve-M meant) is not the right approach. I meant using a mixed-mode assembly. |
That sounds like an interesting approach as well. After having looked at the page you linked, I found an example showing how you would even mix unmanaged C++, C++/CLI, and C# code in one assembly. If you think this through, however, what you are effectively saying is that we don't need a C# wrapper for WinSparkle.dll. Using a mixed-mode assembly would mean we should produce a C++/CLI (i.e., managed) wrapper by essentially replicating the C API defined in |
Thanks for everyone's input, as this is all rather new to me, I may be making mistakes.
Pretty much a noob to .NET but trying to understand what I'm doing wrong with the above steps? |
Could you provide some background on what you are trying to achieve? And what is Using WinSparkle in your .NET solution can be really easy. For example, if you wanted to use the WinSparkle.dll in your C# project, you would just have to declare the WinSparkle API functions in a static wrapper class or in the class that is using those API functions. For example:
Assuming you added WinSparkle.dll as I described before, you could then use those static methods in your code like so:
Just putting WinSparkle.dll somewhere doesn't do anything, unless you put it into the correct output folder. If you manually put it into the output folder, though, you'd have to do that again once you clean the solution, for example. Instead of writing your own wrapper, you can certainly use an existing wrapper like the one proposed in this PR. I've not worked with it and don't know whether it includes a project that you can just launch to play with WinSparkle. |
@ThomasBarnekow I'm using this PR as the basis. I edited my question to reflect the proper naming My above attempt is to get this PR up and running using
-- edit -- |
@FrenchBen OK, here is what you should ensure (going bottom-up):
Having said that, I have not looked at this PR in detail and don't know whether there are any issues that might contribute to your problem. |
Moderation note: Please let’s limit the discussion under this PR to the subject of review of this PR only. @Herve-M: Let me know when you think it’s merge-ready (since you’re still making changes, I assume it’s not) or when there’s something you want me to look at — GitHub doesn’t send email notifications for pushed changes, only for added comments, so I didn’t even realize there were new commits until the recent commentstorm. (Organizing (and describing) the commits as @GitCop says in the linked guide would be nice w.r.t. ease of reviewing, but not necessary — this is a very important PR, I’m OK with squashing the branch myself.) |
[DllImport("WinSparkle.dll", EntryPoint = "win_sparkle_set_appcast_url", CharSet = CharSet.Ansi, CallingConvention = CallingConvention.Cdecl)] | ||
public static extern void SetAppcastUrl( | ||
[param: MarshalAs(UnmanagedType.AnsiBStr)] string url | ||
); |
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 do you marshal as UnmanagedType.AnsiBStr
if the WinSparkle API is declared as follows?
WIN_SPARKLE_API void __cdecl win_sparkle_set_appcast_url(const char *url)
Using UnmanagedType.LPStr
would be a better fit. An UnmanagedType.AnsiBStr
is a COM-style BSTR with a prefixed length and ANSI characters.
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'll add that Thomas pointed this already previously in another instance — when such an issue is found, you need to go through all such bugs, not just the one commented on.
@vslavik, @Herve-M, has this PR been abandoned? I am asking because I could contribute a solution or project that provides a managed wrapper for the native WinSparkle.dll that:
I needed that for Microsoft Office add-ins that would normally be used by 32-bit Office but sometimes also with 64-bit Office. And I wanted to avoid building platform-specific assemblies. The solution uses two classes, one called WinSparkle and another one called WinSparkleWrapper. The first one is meant to be used by managed callers. The second one is meant to describe the native DLL. Pointing to the 32-bit native DLL merely as an example, WinSparkleWrapper looks like this:
The WinSparkle class uses delegates that are dynamically built based on the above description of the native methods. For example:
Managed code would simply use the WinSparkle class (not the WinSparkleWrapper) as follows:
The question is whether you would be interested in this contribution and, if so, how it should be integrated. The issue starts with my using of Visual Studio 2017, with which I cannot even build WinSparkle right now (tried the solution for VS2015), and my C/C++ skills are a bit rusty (last significant development in C/C++ in 1994). |
@ThomasBarnekow Please make a separate issue or PR if you wish to discuss your code unrelated to this PR (except for the general subject) — this is not the place.
It seems pretty clear that @Herve-M has no interest in it anymore, yes. |
@ThomasBarnekow This PR can be considered as abandoned, I will close it. |
@vslavik, I have seen that there is at least one NuGet package (see WinSparkle.Net) and corresponding GitHub project (see Sparkle.Net) that already provides a solution like mine (different implementation but same outcome). Thus, I think it doesn't make sense to spend time on this. |
This PR take part of solving issue #20.
It keep the way project where organized.
And try to correct most of the problems from pull #55.
This don't add the feature as unique DLL for C#. This will come after, if this is validated.
Target WinSparkle API 0.5.X (from master)