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

Add support for Windows on Arm64 (WoA). #559

Merged

Conversation

chirontt
Copy link
Contributor

@chirontt chirontt commented Mar 21, 2024

Add support for Windows on Arm64 (WoA)

A new launcher fragment for WoA is added: org.eclipse.equinox.launcher.win32.win32.aarch64

Various features are also updated to include the new fragment.

To manually build the Equinox launcher binaries for WoA

On a WoA box, run the following command at the root directory of this repo:

mvn clean generate-resources -Dnative=win32.win32.aarch64

and the following launcher binaries for WoA are generated:

eclipse.exe
eclipse_11900.dll
eclipsec.exe

which are moved over to the rt.equinox.binaries repo which is supposedly at the same directory level as that of this repo, i.e. at ..\rt.equinox.binaries, and to their corresponding sub-directories there as followed:

..\rt.equinox.binaries\org.eclipse.equinox.executable\bin\win32\win32\aarch64\eclipse.exe
..\rt.equinox.binaries\org.eclipse.equinox.executable\bin\win32\win32\aarch64\eclipsec.exe
..\rt.equinox.binaries\org.eclipse.equinox.launcher.win32.win32.aarch64\eclipse_11900.dll

@laeubi
Copy link
Member

laeubi commented Mar 21, 2024

@chirontt thanks for bringing this forward, in SWT we used (eclipse) linked folders to make the same sources available for different fragements, would something like this be suitable for the WoA fragemnt as well so we don't need an additional intermediate?

Copy link

github-actions bot commented Mar 21, 2024

Test Results

0 files   -   660  0 suites   - 660   0s ⏱️ - 1h 10m 32s
0 tests  - 2 195  0 ✅  - 2 148  0 💤  -  47  0 ❌ ±0 
0 runs   - 6 729  0 ✅  - 6 586  0 💤  - 143  0 ❌ ±0 

Results for commit 1e73236. ± Comparison against base commit a394f9c.

♻️ This comment has been updated with latest results.

@chirontt
Copy link
Contributor Author

Yes, it may be doable. The benefits will be to remove the new org.eclipse.equinox.security.win32 fragment (as not needed) and keep the existing fragment org.eclipse.equinox.security.win32.x86_64 intact, while the new fragment for WoA org.eclipse.equinox.security.win32.aarch64 will share the existing Java & C source code in the x86_64 folder by making use of the source.. = property in the build.properties file to link to them. Is this what you mean, just to be sure?

@chirontt
Copy link
Contributor Author

@laeubi I've done what you suggested, but I encounter some small hiccup: how to include the cpp source files in the final source bundle (like what the x86_64 counterpart is doing)? Here are my various attempts for the build.properties for this org.eclipse.equinox.security.win32.aarch64 module (after removing all unrelated stuff for clarity): the 1st attempt which I consider the simplest and matching closely with the x86_64 counterpart:

source.. = ../org.eclipse.equinox.security.win32.x86_64/src/
src.includes = ../org.eclipse.equinox.security.win32.x86_64/cpp/

but the resulting source bundle won't include the cpp files, as if the src.includes property just ignores the above setting. Then I try these:

source.. = ../org.eclipse.equinox.security.win32.x86_64/src/,\
           ../org.eclipse.equinox.security.win32.x86_64/cpp/

but then the resulting binary (jar) bundle would include those cpp source files too. The next try are these settings:

source.. = ../org.eclipse.equinox.security.win32.x86_64/src/,\
           ../org.eclipse.equinox.security.win32.x86_64/cpp/
bin.excludes = jnicrypt.*

and it almost achieves what I want, but the resulting source bundle won't have a cpp subfolder for those source files (they are left in the root of the bundle), unlike what the source bundle for x86_64 is currently having.

Is there a better way? Otherwise I'd use the bin.excludes way for the PR then.

@HannesWell
Copy link
Member

@chirontt I also looked into this and in order to simplify the org.eclipse.equinox.security.win32 I think it is worth investigating if we can just use JNA in order to perform the only native call. Just like it is done for Mac and Linux. If that works, we can then just use the same fragment for x86_64 and aarch64.

Would you be interested in looking into this in a separate PR? If not, I can do it.

@chirontt
Copy link
Contributor Author

@HannesWell Yes, using JNA is a good idea, better than these clunky JNI/C files with re-compile for various platforms. But I have no experience with JNA, so please go ahead and see if JNA is applicable here. Once JNA is proven to work, there are other places in eclipse.platform repo to apply it as well (if possible):

https://github.com/eclipse-platform/eclipse.platform/tree/master/resources/bundles/org.eclipse.core.filesystem/natives
https://github.com/eclipse-platform/eclipse.platform/tree/master/resources/bundles/org.eclipse.core.resources/natives

@HannesWell
Copy link
Member

HannesWell commented Mar 24, 2024

Yes, using JNA is a good idea, better than these clunky JNI/C files with re-compile for various platforms. But I have no experience with JNA, so please go ahead and see if JNA is applicable here.

Absolutely. It looks good, see #564.

Once JNA is proven to work, there are other places in eclipse.platform repo to apply it as well (if possible):

https://github.com/eclipse-platform/eclipse.platform/tree/master/resources/bundles/org.eclipse.core.filesystem/natives
https://github.com/eclipse-platform/eclipse.platform/tree/master/resources/bundles/org.eclipse.core.resources/natives

The filesystem-access implemented in org.eclipse.core.filesystem is very performance sensitive and I'm not sure if migrating it to JNA would be a performance regression for x86_64. As far as I know performance is not the No.1 priority for JNA. So that would have to be measured first.
But as written in eclipse-platform/eclipse.platform.releng.aggregator#577 (comment), the default-impl should already work for WoA (IIRC it is about 10% slower, which is often not dramatic, but nevertheless not acceptable for some users).
Eventually we should try to migrate the native access to the new Foreign Function and Memory Access API finalized in Java-22, but it will probably take some time until we can use that (latest with the next LTS Java-25, which will be usable for Eclipse in about two years). So I'm not sure if it is worth the effort to have that intermediate step.

The situation for org.eclipse.core.resources.win32 is similar. Without it you (probably) only loose refreshing out resources changed outside the workspace through native polling. That is probably not so performance sensitive, but a lot more work to migrate (equinox.security.win32 is even much simplified by using JNA) so it is also the question if it is worth the effort to migrate it to something else than FFM.

Therefore my suggestion is to first focus on the absolute requirements, namely SWT and the Equinox-Launcher.
SWT is on track, but for the Equinox-Launcher (i.e. this change), we also have to adjust the build-pipeline to build the executable.
Unlike SWT the pipeline is not yet migrated to a nice Jenkins pipeline stored in this git-Repo and still exists as manually configured Jenkins builds in https://ci.eclipse.org/releng/view/Launcher/.
I had the intention to migrate that too for a while but will take this as an opportunity to really do it. Once that work is done and with the work done for SWT to support WoA, it should be as simple as the adjustments in the SWT build-pipeline to land this.
Once the basics work and we have even have integration-tests for WoA we can still think about porting o.e.core.filesystem/resources.

@HannesWell
Copy link
Member

@chirontt please rebase this PR and resolve the conflicts. The changes to the equinox.security fragments should not be necessary anymore since #564 is submitted.

@chirontt chirontt force-pushed the equinox_windows_on_arm64 branch from bf27f2a to 196f9c4 Compare March 27, 2024 00:12
@chirontt
Copy link
Contributor Author

Branch updated with the latest from master, with PR description updated as well.

@HannesWell
Copy link
Member

@chirontt I have now created #575 in order to track the work on streamlining the Equinox launcher+executable build pipeline. Once that is done, it should be simple to complete this.

@HannesWell
Copy link
Member

@chirontt I have now created #575 in order to track the work on streamlining the Equinox launcher+executable build pipeline. Once that is done, it should be simple to complete this.

Finally I have a draft PR for that up: #603
It will need some adjustments in the Jenkins configuration done by the EF-infra structure team and some testing, but since it is relativly similar to the SWT natives build pipeline I assume that all of that is completed relatively soon.

In the meantime you could already rebase your PR and resolve the conflicts due to my earlier rework/clean-up of the native build.

@chirontt chirontt force-pushed the equinox_windows_on_arm64 branch from ba48051 to 909903a Compare May 4, 2024 02:31
@chirontt
Copy link
Contributor Author

chirontt commented May 4, 2024

In the meantime you could already rebase your PR and resolve the conflicts due to my earlier rework/clean-up of the native build.

PR rebased to latest in master, and PR description is updated as well to reflect the mvn command now used for building the launcher binaries.

@HannesWell HannesWell force-pushed the equinox_windows_on_arm64 branch 2 times, most recently from 0f78456 to 17266db Compare May 6, 2024 23:22
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

Thanks for the update. Now that #603 is submitted too, I updated this PR to also make the necessary modifications to the Jenkinsfile (similar like the one done in SWT).

If everything goes well, I'll plan to submit this tomorrow and continue with the other related pending PRs.

pom.xml Outdated Show resolved Hide resolved
@HannesWell HannesWell force-pushed the equinox_windows_on_arm64 branch 2 times, most recently from 63e2573 to d05a80e Compare May 7, 2024 17:07
A new launcher fragment for WoA is added:
'org.eclipse.equinox.launcher.win32.win32.aarch64'

Various features are also updated to include the new fragment.

To build the Equinox launcher binaries for WoA:

On a WoA box, run the following command at the root directory of this
repo:

mvn clean generate-resources -Dnative=win32.win32.aarch64

and the following launcher binaries for WoA are generated:

eclipse.exe
eclipse_11902.dll
eclipsec.exe

which are moved over to the 'equinox.binaries' repo which is
at the same directory level as that of this repo, i.e.
'..\equinox.binaries',
and to their corresponding directories there as followed:

..\equinox.binaries\org.eclipse.equinox.executable\bin\win32\win32\aarch64\eclipse.exe
..\equinox.binaries\org.eclipse.equinox.executable\bin\win32\win32\aarch64\eclipsec.exe
..\equinox.binaries\org.eclipse.equinox.launcher.win32.win32.aarch64\eclipse_11902.dll
@HannesWell HannesWell force-pushed the equinox_windows_on_arm64 branch from d05a80e to 1e73236 Compare May 7, 2024 17:59
Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

I just applied the suggested changes and fixed a small missing piece in the org.eclipse.equinox.executable.feature\resources\build.xml (although I'm not sure if that's even used anymore, see #592).

With that this is ready for submission, once the Jenkins build has succeeded (all other builds cannot succeed without the initial build of the win32.aarch64 launcher binaries).

Thank you for yet another great contribution and an important step towards full support of WoA by Eclipse.

@HannesWell HannesWell merged commit 030120b into eclipse-equinox:master May 7, 2024
22 of 27 checks passed
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