Rewrite the native code to JNA #365
Conversation
(cherry picked from commit 0f82448)
(cherry picked from commit f107221)
(cherry picked from commit 5a76a6c)
(cherry picked from commit c63d335)
(cherry picked from commit 7b4787b)
(cherry picked from commit 303c48b)
(cherry picked from commit a7e7478)
(cherry picked from commit cf8870a)
(cherry picked from commit e1cdbc4)
(cherry picked from commit 19c8129)
(cherry picked from commit 754d04e)
(cherry picked from commit 5305e40)
(cherry picked from commit f9209a0)
(cherry picked from commit 3a9c274)
(cherry picked from commit 0139ceb)
(cherry picked from commit 018dbf7)
(cherry picked from commit 490ce95)
(cherry picked from commit 8e6aeea)
(cherry picked from commit 4047629)
(cherry picked from commit d935dda)
(cherry picked from commit 3f2877e)
They all were replaced by backend's JNA invocations. (cherry picked from commit a44f466)
(cherry picked from commit 122c1a0)
It now only tests mutex, because only mutex is used in the actual code. (cherry picked from commit ac791b2)
Only mutices are used by the TFS client. (cherry picked from commit 0845e5a)
(cherry picked from commit ca7d07c)
(cherry picked from commit eb2a0a8)
(cherry picked from commit 1e89b8a)
(cherry picked from commit e1087fb)
We'll move the code to JNA at a later point, for now it should be sufficient as a first step. (cherry picked from commit 5cade3e)
(cherry picked from commit 96ce44d)
(cherry picked from commit be19c73)
(cherry picked from commit 1c20569)
(cherry picked from commit bc1fc9d)
(cherry picked from commit e7ab94e)
(cherry picked from commit 6ced36b)
(cherry picked from commit 98762c1)
(cherry picked from commit e960e00)
(cherry picked from commit be1947b)
(cherry picked from commit bfc1cb5)
(cherry picked from commit 6a9534a)
…mputerName (cherry picked from commit 5c660a2)
(cherry picked from commit b8c95ee)
This is supposed to fix a problem running on M1 Mac hardware. (cherry picked from commit fdcfb8b)
(cherry picked from commit ecc9751)
(cherry picked from commit b7a540d)
|
Thanks for putting this together. Is it safe to assume that the commits are standalone? That is, if I were to start cherry picking them one by one, would I retain a working state. I can't think of another way to go about this with the changes being so extensive. Also, do you know if the unit tests cover the changes? If no, how can I be sure any changed code is executed? I don't get any hints when using the plugin that the native code is exercised. |
I believe there are several bunches of commits, replacing the native modules one by one — and inside a single bunch the state may not be stable. But it is in-between the bunches. And there are a couple of fresh bug fixes on top of everything else (among the latest 5 commits).
Yes, they do. I am not sure how exhaustive the tests currently are, but they cover quite a lot. Sorry, my memories are pretty vague since most of the changes are from 2019. One known fact is that they don't cover everything though. After we've added integration tests with Azure DevOps IntelliJ, several new critical issues were discovered (the ones that were causing the process to crash, being implemented in native code and all that). All the fixes for these issues are included in this PR, even though the integration test infrastructure is not included (I have only recently ported a bunch of files from the Azure DevOps IntelliJ repo to our fork of team-explorer-everywhere). |
|
Would it be possible to submit the bunches one by one? For at least the first few, I will need some time to wrap my head around this. I would only need the first bunch for now. |
|
After thinking a bit on this, I believe yeah, it should be possible. Not sure if we'll have to resolve same similar conflicts again one by one though, but we can try at least. These are the pull requests from ( |
|
You mentioned that there were some problems that were later fixed. Are you able to point out which PRs had such bugs and which commits or PRs resolved? I could look at merging the PRs one at a time on a branch. I'll see if I can find time to get that started later this week. |
|
All the "problems that were later fixed" are included into the scope I outlined ( |
|
If you prefer me to send the smaller PRs myself, then please tell me. |
|
It would definitley save me some time to have you submit small PRs. You would only need to submit one at a time. I need to do some tests by integrating with my dev env after each step. |
|
Alrighty, let's go. There will be about 19 PRs (I'll see if it's possible to merge some of them together). I have outlined the plan in the top message of this PR, to not miss anything. |
I did my best to extract all the commits related to migration from native binaries to JNA, and put them into this PR, per request from Eric. This repository was forked from an old upstream commit, when you folks declared official deprecation of the repository, so we've had to fork it and support ourselves as part of a joint collaboration effort on Azure DevOps IntelliJ.
Essentially, what I did is remove all the native sources and binaries from the repository, and replaced them with more or less tested JNA implementations; this is why JNA update was needed as well.
This will help to resolve most of the issues related to Apple M1 (and, presumably, Windows Volterra or whatever the ARM64 version of Windows is called these days). No native binaries ⇒ no problems running and loading them. And JNA is thankfully fully portable to all modern CPU architectures.
Some quick notes:
At some point, I've also updated the build environment to Java 8 instead of Java 6, but I'm not sure if this is required for these changes, so I've omitted this change.
One notable feature thrown away during the implementation is the native keychain support for Linux and macOS (see commit cfe2961 in this PR).
I can't remember the exact reason for why I did this, but certainly it was challenging to reimplement. The main client for our usage variant at the time was Azure DevOps IntelliJ plugin (I am a major contributor of), so we didn't need native keychains for it, and they were disabled (the keychain access is handled by the IDE anyway).
Perhaps we'll need to reimplement it before merge.
As you can see, these changes are a bit raw:
they have conflicts with the upstream branch (I believe it's possible to resolve, though I am not an expert in Eclipse build system, so it's not easy for me1)
the disabled native keychains, as mentioned above, may be blocker
I have to be honest: since building the upstream repository is quite challenging (you need to set up Eclipse etc. etc.), I did not even check if this branch builds. We have an automatic build system set up in the fork repository, but it is absent in the upstream for now.
If you wish to proceed with such radical changes, I am of course ready to investigate and fix the build errors, if any.
@eric-milles, how would you like us to proceed with these changes?
For me, an ideal solution would be if you help us to merge the upstream into this branch, and find resources to reimplement the native keychains back.
Also, all the native components have automated tests, but again, it's quite hard to run these tests on a modern system2, so I'll have to rely on your ability to run them.
Contained PRs
#4: Remove native code for JNI console #366#5#6#7#8#9#10#11#12#13#14#15(note version increment)#16#17#18#19#20#21#29Footnotes
In the fork repository, we've started migration to a more modern build approach, but this is still in progress, takes a lot of time and a bit stalled effort for now. Besides, it is out of scope of this PR. ↩
JetBrains fork of this repository has set up an automated test suite using GitHub Actions these days, though it's out of scope of this PR. ↩