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

COM support: Added significant enhancements. #397

Closed
wants to merge 51 commits into from

Conversation

dhakehurst
Copy link

Added significant enhancements to the COM support.
Most of this is under the com.sun.jna.platform.win32.COM.util package, with a few additions to the com.sun.jna.platform.win32.COM package.

there are tests added (which I think cover all the added enhancements), plus we have now been using it extensively for 3 months.

Added proxy and annotations to assist easy use of COM.
Added java friendly version of some COM interfaces.
Added Tests.
COM threads for different COM object access.
Explicit termination of COM thread necessary, finalizer use is not
recommended as reliable.
Corrected signature (IID -> REFIID) in IDispatch.
… in dispatch.c, the parameter is used under WIN32 build.
possible.
Required a change to Variant, to enable size of array of VariantArg to
be set. Might be a cleaner way to do this, but it could break the
existing API of VariantArg.
updates to handling callback arguments
@dblock
Copy link
Member

dblock commented Feb 9, 2015

Looks great, would you please update https://github.com/twall/jna/blob/master/CHANGES.md (i think it needs a "Next" section).

@@ -267,4 +267,62 @@ HRESULT CoCreateInstance(GUID rclsid, Pointer pUnkOuter, int dwClsContext,
*/
void CoTaskMemFree(Pointer pv);

/**
* Retrieves a pointer to the default OLE task memory allocator
Copy link
Member

Choose a reason for hiding this comment

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

Missing a period :)

Copy link
Author

Choose a reason for hiding this comment

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

added a period :-)

@dblock
Copy link
Member

dblock commented Feb 9, 2015

This is some nice solid work here, @dhakehurst, hats off. I made a couple of cosmetic comments, feel free to do nothing about them. I will be happy to merge after the CHANGES update.

@dhakehurst
Copy link
Author

Hi,
Not quite sure what to modify in the CHANGES.md file!

@dblock
Copy link
Member

dblock commented Feb 10, 2015

I think CHANGES needs something like what you wrote above:

  • Use of interfaces and annotations to provide easier implementation of COM interfaces (uses InvocationHandler).
  • Support for COM event callbacks (this was particularly tricky, very happy I got it to work).
  • Support for COM interface discovery by iteration over the RunningObjectTable.

Whatever else you want to write, maybe sprinkle it with specific class names since not everybody uses Windows (eg. Added com.sun.jna......).

I think you might want to squash these commits, there's a lot of small stuff going on, it will be hard to track history. However I don't feel strongly about this, you decide.

I don't think the eclipse or maven project should be committed. For one I personally prefer to track it outside of JNA with my own settings. Please remove it. Unless @twall feels otherwise. Or we can discuss adding those in a separate PR?

@dblock
Copy link
Member

dblock commented Feb 13, 2015

@dhakehurst LMK if you need help.

added changes to CHANGES.md
(fixed a bug in ProxyObject)
@dhakehurst
Copy link
Author

Have added some text to the CHANGES file.
I need the maven build in my version of the repo, no idea how to mark it not to be included in the pull request!

@dblock
Copy link
Member

dblock commented Feb 20, 2015

Can you tell us why you need the maven stuff? I understand you might prefer Maven, but this project doesn't use it to build.

To exclude it:

  1. Move the maven folder(s) out of the way (like cut and paste elsewhere).
  2. Add maven to .gitignore, git add --all and git commit (as if the maven folder was git rm-ed).
  3. Put the maven folder back, it won't be tracked since it's in .gitignore.

@dblock
Copy link
Member

dblock commented Feb 20, 2015

And thanks :) This is a LOT of stuff, I am willing to help in any way I can to get this in.

@@ -7,6 +7,10 @@ Release 4.2

Features
--------
* Significant enhancements to COM support under com.sun.jna.platform.win32.COM.util, - [@dhakehurst](https://github.com/dhakehurst)
Copy link
Member

Choose a reason for hiding this comment

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

You have an extra , at the end of this line before your name.

@dhakehurst
Copy link
Author

The maven stuff is used by our Jenkins server to do the build. It need to be maven so that it integrates with the rest of the jenkins builds (up/down stream stuff).

Are you sure that no one else would be interested in maybe building with maven? Is it very wrong to have an alternative build approach in the repository? (it doesn't yet build the native stuff, but it could do with a bit more work.)

@dblock
Copy link
Member

dblock commented Mar 4, 2015

@twall Can comment about maven, but I am 99% sure, we have hard time maintaining one build mess ;) If you want to suggest building with maven I would suggest splitting it into a separate PR.

@twall
Copy link
Contributor

twall commented Mar 4, 2015

While I’m not against a more mavenized build, the one we’ve got is a bit of a pain (most likely due to the three of us who usually do releases are not well versed in that space, or at least the providers we’re using).

We just need to be clear about what we’re getting out of it and that we’re able to maintain it.

I do all builds with ant (including all the native builds), then when time comes to put out a release either db, myself, or drollo will follow the maven release steps to get it out to the release repo. I’d love to have a more push-button approach to releases, though.

I agree that any maven build proposal should be in its own PR.

Thanks again for this COM work!

On Mar 4, 2015, at 6:23 AM, Daniel Doubrovkine (dB.) @dblockdotorg [email protected] wrote:

@twall Can comment about maven, but I am 99% sure, we have hard time maintaining one build mess ;) If you want to suggest building with maven I would suggest splitting it into a separate PR.


Reply to this email directly or view it on GitHub.

@dhakehurst
Copy link
Author

I am not suggesting you move jna to maven.

However, I need the maven stuff because we build with maven. Is there a way for me to keep the maven folder in our clone of the repo, but not export it to you in the PR? There is none I know of.

Can I suggest that you simply exclude the maven folder when you do the merge. It is totally self contained, nothing outside it references into it.

@twall
Copy link
Contributor

twall commented Mar 4, 2015

That’s acceptable. We can either do the merge manually or remove the maven folder after the merge.

On Mar 4, 2015, at 7:29 AM, Dr. David H. Akehurst [email protected] wrote:

I am not suggesting you move jna to maven.

However, I need the maven stuff because we build with maven. Is there a way for me to keep the maven folder in our clone of the repo, but not export it to you in the PR? There is none I know of.

Can I suggest that you simply exclude the maven folder when you do the merge. It is totally self contained, nothing outside it references into it.


Reply to this email directly or view it on GitHub.

@dblock
Copy link
Member

dblock commented Mar 4, 2015

I'll take care of the merge, but I will need some time to get to this.

@bhamail
Copy link
Contributor

bhamail commented Mar 4, 2015

There are some known problems with the existing ant build (along the lines of dependent targets not working as expected “depending” on how/when invoked). This has lead to incorrect binaries being released in the past. I mention this because I’m open to any solution to that sort of problem (and a maven build might be one). I’ve thought about doing a fully mavenized build before, but I couldn’t see a clean way to further modularize the various native builds w/out a bunch of “moves” and/or kludges. I’m interested to hear more about how you’re approaching this.

Dan

On Mar 4, 2015, at 6:49 AM, Timothy Wall [email protected] wrote:

While I’m not against a more mavenized build, the one we’ve got is a bit of a pain (most likely due to the three of us who usually do releases are not well versed in that space, or at least the providers we’re using).

We just need to be clear about what we’re getting out of it and that we’re able to maintain it.

I do all builds with ant (including all the native builds), then when time comes to put out a release either db, myself, or drollo will follow the maven release steps to get it out to the release repo. I’d love to have a more push-button approach to releases, though.

I agree that any maven build proposal should be in its own PR.

Thanks again for this COM work!

On Mar 4, 2015, at 6:23 AM, Daniel Doubrovkine (dB.) @dblockdotorg [email protected] wrote:

@twall Can comment about maven, but I am 99% sure, we have hard time maintaining one build mess ;) If you want to suggest building with maven I would suggest splitting it into a separate PR.


Reply to this email directly or view it on GitHub.


Reply to this email directly or view it on GitHub #397 (comment).

@dblock
Copy link
Member

dblock commented Mar 16, 2015

I have merged the code from here minus any maven stuff via d6675fa cause I want to move forward. I got a bunch of failures in the tests, which I will look at and open as bugs - it's very likely that these are things missing from my environment.

Please make small PRs, looking forward to more.

@dblock dblock closed this Mar 16, 2015
@dblock
Copy link
Member

dblock commented Mar 16, 2015

Opened #406, #407 and #408.

mstyura pushed a commit to mstyura/jna that referenced this pull request Sep 9, 2024
…cess#397)

Motivation:

We did not correctly escape chars when shading is used.

Modifications:

- Update jni-utils to fix bug

Result:

Fixes escaping

Related:
- netty/netty-jni-util#13
- netty/netty#12358
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