Open ForNeVeR opened 5 months ago
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.
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 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).
Also, do you know if the unit tests cover the changes?
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 #4
to #21
in the fork repository, and then #29
as well.
(#15
would have to be changed to exclude the version increment, but that's merely a problem I think)
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 (#4
– #21
+ #29
), and by extension into this very PR.
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:
One notable feature thrown away during the implementation is the native keychain support for Linux and macOS (see commit cfe296194a1150e6f852ffa441e1378c0643d97e 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:
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 system[^2], so I'll have to rely on your ability to run them.
[^1]: 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. [^2]: 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.
Contained PRs
#4
: https://github.com/microsoft/team-explorer-everywhere/pull/366#5
#6
#7
#8
#9
#10
#11
#12
#13
#14
#15
(note version increment)#16
#17
#18
#19
#20
#21
#29