Remove short-named equinox.launcher.cocoa.macosx fragment #622

HannesWell · 2024-05-08T22:04:02Z

The full-named org.eclipse.equinox.launcher.cocoa.macosx.x86_64 fragment exists for a long time already and users of the old short-named variant should have had enough time to migrate.

This would also allow to remove the sym-link in the binaries repo:
https://github.com/eclipse-equinox/equinox.binaries/blob/af72d120b237ca99ff37c345e9d0ec20ed49f48a/org.eclipse.equinox.launcher.cocoa.macosx#L1
I wonder why it is pointing to the aarch64 fragment/binaries, altough arch64 support was add about only three years ago, while the short-named fragment exists for over 13 years.

Additionally remove consideration of old development locations of the launcher fragments.

HannesWell · 2024-05-08T22:08:28Z

Thinking about it further, the current state even means that if somebody launches a secondary eclipse (IIRC the removed code-path is not called if the executable is used, which sets the --launcher.library argument to the latest so-file) then a user on mac-x86_64 will eventually use the aarch64 library.
If that's correct, it looks like the launcher libraries for mac can be used on both archs?
Edit: That assumption was wrong, see #622 (comment).

github-actions · 2024-05-08T22:37:41Z

Test Results

660 files + 220 660 suites +220 1h 17m 21s ⏱️ + 27m 39s
2 201 tests ± 0 2 154 ✅ + 1 47 💤 - 1 0 ❌ ±0
6 747 runs +2 249 6 604 ✅ +2 202 143 💤 +47 0 ❌ ±0

Results for commit f3e4a20. ± Comparison against base commit d1356fb.

♻️ This comment has been updated with latest results.

HannesWell · 2024-05-09T20:26:15Z

@tjwatson any objections about this? I didn't find any other reference to org.eclipse.equinox.launcher.cocoa.macosx than the one in Main, which is replaced with this PR too.

The fragment org.eclipse.equinox.launcher.cocoa.macosx is removed in [1] since there are architecture specific replacements now. [1] - eclipse-equinox/equinox#622

HannesWell · 2024-05-09T20:30:49Z

This would also allow to remove the sym-link in the binaries repo:
https://github.com/eclipse-equinox/equinox.binaries/blob/af72d120b237ca99ff37c345e9d0ec20ed49f48a/org.eclipse.equinox.launcher.cocoa.macosx#L1

To do that, I prepared eclipse-equinox/equinox.binaries#4

Phillipus · 2024-05-11T08:44:17Z

If that's correct, it looks like the launcher libraries for mac can be used on both archs?

Do you mean the so files? Each architecture requires its own so compiled for that architecture.

Phillipus · 2024-05-11T08:49:42Z

I didn't find any other reference to org.eclipse.equinox.launcher.cocoa.macosx than the one in Main, which is replaced with this PR too.

As mentioned in #625 it is used when launching a child Eclipse instance on Mac aarch64. Is that perhaps coded somewhere in PDE?

HannesWell · 2024-05-12T18:55:21Z

I didn't find any other reference to org.eclipse.equinox.launcher.cocoa.macosx than the one in Main, which is replaced with this PR too.

As mentioned in #625 it is used when launching a child Eclipse instance on Mac aarch64. Is that perhaps coded somewhere in PDE?

As far as I understand it, in a primary Eclipse the launcher library fragment is computed already in the executable. But if you launch a secondary Eclipse the exe is not involved but the native fragment is determined/searched in the Main.setupJNI().
This can be verified if one places a break point in that method. In a secondary Eclipse this is trivial and in a primary eclipse this can be done by adding the corresponding VM args in the ini or cli Argument (after -vmargs):
https://www.baeldung.com/java-application-remote-debugging#from-java9

I think the problem is that currently in case the os is mac and the arch is x86_64then the fragment without arch suffix is used, which contains (in fact at the moment used to contain see #625) the aarch64 library.
This does not look right to me.
At @Phillipus could you please debug what actually happens in the current state of Main.setupJNI()? E.g. which arch is reported for x86_64 and aarch64 and when the short named fragment is really used respectively if this PR would fix #625?

Phillipus · 2024-05-12T18:59:26Z

@HannesWell See #625 (comment)

Phillipus · 2024-05-12T19:01:01Z

Sorry, I just read this:

if this PR would fix #625?

Will check the PR tomorrow!

Phillipus · 2024-05-12T19:07:19Z

@HannesWell Actually I just did a quick smoke test:

Copied your Main code from your PR to my local copy
Deleted the org.eclipse.equinox.launcher.cocoa.macosx folder from my target bundle pool (workspace/.metadata/.plugins/org.eclipse.pde.core/.bundle_pool/plugins/ but left the org.eclipse.equinox.launcher.cocoa.macosx.aarch64 folder there
Launched child RCP app - splash screen present and correct!

HannesWell · 2024-05-12T19:22:05Z

Thank you for the fast check and the reference.
I think we should definitely have the code change in the Main class since it fixes #625. Of course we could also fully restore the old content but adjusting the main class in general looks like a step forward.

Regarding the removal of the org.eclipse.equinox.launcher.cocoa.macosx fragment, this was also tried in the past e.g.:
https://bugs.eclipse.org/bugs/show_bug.cgi?id=572799#c14
@sravanlakkimsetti can you please explain what exactly failed in adjustmentimg P2?

What also confuses me in general:
I assume the launcher Mac fragment without arch initially used to target x86_64? But now it targets aarch64? Isn't that a problem?

Phillipus · 2024-05-12T19:27:43Z

From my own usage on aarch64 and x86_64 there are two scenarios:

Launch primary Eclipse or RCP app
Launch secondary Eclipse or RCP app

On x86_64 (1) and (2) use the org.eclipse.equinox.launcher.cocoa.macosx.x86_64 bundle. No problem.
On aarch64 (1) uses org.eclipse.equinox.launcher.cocoa.macosx.aarch64 and (2) uses org.eclipse.equinox.launcher.cocoa.macosx

AFAICT your PR should resolve this discrepancy.

Phillipus · 2024-05-12T19:29:38Z

To be clear - on my Mac x86_64 machine there is no org.eclipse.equinox.launcher.cocoa.macosx bundle in the bundle pool, only the org.eclipse.equinox.launcher.cocoa.macosx.x86_64 one

HannesWell · 2024-05-12T19:32:05Z

Copied your Main code from your PR to my local copy

Could you please also check what the value of the arch variable is? Actually the if condition can only be true if arch has the value x86_64? But then it uses an aarch64 fragment?

Phillipus · 2024-05-12T19:34:57Z

Could you please also check what the value of the arch variable is?

Here:

    private String getArch() {
        if (arch != null)

arch is aarch64

Phillipus · 2024-05-12T19:48:35Z

Tested on x86_64 machine launching RCP app from Eclipse and splash screen is showing OK.

Phillipus · 2024-05-13T08:03:14Z

I assume the launcher Mac fragment without arch initially used to target x86_64? But now it targets aarch64? Isn't that a problem?

It's not a problem on x86_64 because it's not present on x86_64 installations or the target bundle pool. It is only present on aarch64 and is a redundant duplicate, used when launching a child instance.

Phillipus · 2024-05-13T09:43:46Z

AFAICT this PR should just work. Is it possible to create a build to test it? Or maybe just commit and I can test the I-build on x86_64 and aarch64.

sravanlakkimsetti · 2024-05-13T11:50:10Z

@HannesWell

Hard link org.eclipse.equinox.launcher.cocoa.macosx was added as a workaround during aarch64 support work.
During the full SDK build p2 builder was looking for org.eclipse.equinox.launcher.cocoa.macosx for aarch64 architecture though there was no reference.

When I investigated at that time I found there is a hard coding in p2 builder for mac to look for mac universal binaries/libraries(meaning org.eclipse.equinox.launcher.cocoa.macosx fragment). Only in case of x86_64 we had a code to look for org.eclipse.equinox.launcher.cocoa.macosx.x86_64 fragment.

I could not find the exactly where to change so as a stop gap I added hard link org.eclipse.equinox.launcher.cocoa.macosx pointing to org.eclipse.equinox.launcher.cocoa.macosx.aarch64

Please do check whether you are able to do a full SDK build locally. if the SDK build works we can go ahead with this changes.

HannesWell · 2024-05-13T17:48:23Z

I assume the launcher Mac fragment without arch initially used to target x86_64? But now it targets aarch64? Isn't that a problem?

It's not a problem on x86_64 because it's not present on x86_64 installations or the target bundle pool. It is only present on aarch64 and is a redundant duplicate, used when launching a child instance.

Yes, my confusion was because I miss-understood the braces in

equinox/bundles/org.eclipse.equinox.launcher/src/org/eclipse/equinox/launcher/Main.java

Line 453 in fadf6d1

    
           if (!(fragmentOS.equals(Constants.OS_MACOSX) && !Constants.ARCH_X86_64.equals(fragmentArch))) {

With negations pushed down the condition actually is
!fragmentOS.equals(Constants.OS_MACOSX) || Constants.ARCH_X86_64.equals(fragmentArch), but I falsely read it as
!fragmentOS.equals(Constants.OS_MACOSX) && !Constants.ARCH_X86_64.equals(fragmentArch) and therefore falsely concluded that only for mac and x86_64 arch the short name is used, but actually it used the fragment without arch for mac only if the arch is NOT x86_64. Thinking about it again, even with the falsely read condition my conclusion was logically wrong...

Furthermore it's not the executable that searches for the launcher's native fragment, it is recorded in the ini file (or specified as CLI argument) via the --launcher.library argument. For example in the current I-build Eclipse SDK for Windows x86_64 it is:

--launcher.library
plugins/org.eclipse.equinox.launcher.win32.win32.x86_64_1.2.1000.v20240507-1834

In my Oomph provisioned SDK it is an absolute path to that fragment located in my User bundle-pool and for the Eclipse SDK for mac-aarch64 it is:

--launcher.library
../Eclipse/plugins/org.eclipse.equinox.launcher.cocoa.macosx.aarch64_1.2.1000.v20240507-1834

Please do check whether you are able to do a full SDK build locally. if the SDK build works we can go ahead with this changes.

Thank you Sravan for the elaboration, with that and my confusion from above resolved I now understand what is going on.

And found the following locations that need adjustment:

IIRC also also saw such pattern in PDE-build but I can't find it anymore.

All these locations could be changed to use the full-named launcher fragment in all cases, but I think it would be best if the requirement informations are encoded in the main launcher bundle or its fragments.
For example in case we have only one fragment that supports all archs (IIRC this is possible for mac binaries or if the FFM API is used) or if no fragments are nessary at all anymore (looking at #623), then all these places have to be adjusted again and old and new versions have to be supported for some time, making it even more complex.
Therefore it would be ideal if there would also be a requirement of the launcher host to the corresponding fragment in the environment, similar it is suggested in eclipse-platform/eclipse.platform.swt#490.
The launcher code is executed before the OSGi runtime starts but such metadata would feed P2's requirement and would also allow to determine the requirements in a target-platform.

AFAICT this PR should just work. Is it possible to create a build to test it? Or maybe just commit and I can test the I-build on x86_64 and aarch64.

I have now created #629 to only apply the changes in the Main class to first of all address #625.

Eventually removing the org.eclipse.equinox.launcher.cocoa.macosx will probably take a bit longer and probably won't happen in this release since the P2 changes have to trickle down into Tycho first.

Phillipus · 2024-05-13T17:54:01Z

Eventually removing the org.eclipse.equinox.launcher.cocoa.macosx.x86_64 will probably take a bit longer

You mean org.eclipse.equinox.launcher.cocoa.macosx? If so, that currently now has no so file in it so it's redundant.

HannesWell · 2024-05-13T17:54:43Z

Eventually removing the org.eclipse.equinox.launcher.cocoa.macosx.x86_64 will probably take a bit longer

You mean org.eclipse.equinox.launcher.cocoa.macosx? If so, that currently now has no so file in it so it's redundant.

You are right. Sorry, Copy-paste-mistake.

For for Mac OS X there was no equinox-launcher fragment for CARBON with full name, i.e. with <ws>.<os>.<arch> suffix, but only a org.eclipse.equinox.launcher.carbon.macosx fragment. But since Eclipse 4.2, released 2012, CARBON is not supported anymore at all [1]. Therefore the code adjust to that special naming schema, which also imposed difficulties or new supported architectures on Macos should be removed. See also eclipse-equinox/equinox#622 [1] - https://download.eclipse.org/eclipse/updates/4.2/R-4.2-201206081400

AbstractArtifactDependencyWalker The AbstractArtifactDependencyWalker is only used to determine the build-qualifier of of features, products and repositories and for that should only consider what's contained in those assembly-artifacts. It therefore should not add any special fragments to the set of considered artifacts. This also helps to simplify the situation for equinox-launcher fragments for Mac, where still a special treatment for CARBON API artifacts exists, although the org.eclipse.equinox.launcher.carbon.macosx artifact is not supported anymore since 2012 [1]. Required for eclipse-equinox/equinox#622 [1] - https://download.eclipse.org/eclipse/updates/4.2/R-4.2-201206081400

The fragment org.eclipse.equinox.launcher.cocoa.macosx is removed in [1] since there are architecture specific replacements now. [1] - eclipse-equinox/equinox#622

The full-named org.eclipse.equinox.launcher.cocoa.macosx.x86_64 fragment exists for a long time already and users of the old short-named variant should have had enough time to migrate. And remove consideration of old development locations of the launcher fragments.

HannesWell · 2024-09-19T23:22:06Z

Good news:
With Tycho 4.0.9 the complete aggregator build has now passed with this and eclipse-equinox/equinox.binaries#4:

Test cocoa.macosx launcher fragment removal eclipse-platform/eclipse.platform.releng.aggregator#2224

So this can finally be submitted.

This also means that from 2024-12 on-wards Eclipse products for Mac can only be build with Tycho 4.0.9 or later (no clue the state of ancient versions).
But I think that's acceptable.

HannesWell requested a review from tjwatson May 8, 2024 22:04

HannesWell force-pushed the remove-launcher.macosx-fragment branch from 1111892 to 0c44c0e Compare May 8, 2024 22:10

HannesWell force-pushed the remove-launcher.macosx-fragment branch from 0c44c0e to 740aba9 Compare May 9, 2024 19:45

HannesWell mentioned this pull request May 9, 2024

Remove org.eclipse.equinox.launcher.cocoa.macosx link eclipse-equinox/equinox.binaries#4

Open

HannesWell mentioned this pull request May 10, 2024

Child app launched from Eclipse no longer shows splash screen since I20240507-2220 build (Mac aarch64) #625

Closed

HannesWell force-pushed the remove-launcher.macosx-fragment branch from 740aba9 to 2d4ae1e Compare May 13, 2024 17:53

HannesWell changed the title ~~Remove short-named equinox.launcher.cocoa.macosx fragment and its usage~~ Remove short-named equinox.launcher.cocoa.macosx fragment May 13, 2024

HannesWell mentioned this pull request May 19, 2024

Always use full-named o.e.equinox.launcher artifact eclipse-tycho/tycho#3860

Closed

HannesWell mentioned this pull request May 19, 2024

Always use full-named o.e.equinox.launcher artifact eclipse-equinox/p2#519

Merged

HannesWell mentioned this pull request May 20, 2024

Remove equinox-launcher fragments handling in AbstractArtifactDependencyWalker eclipse-tycho/tycho#3862

Merged

HannesWell force-pushed the remove-launcher.macosx-fragment branch from 2d4ae1e to a4557ef Compare August 14, 2024 21:08

HannesWell mentioned this pull request Aug 14, 2024

Test cocoa.macosx launcher fragment removal eclipse-platform/eclipse.platform.releng.aggregator#2224

Draft

HannesWell force-pushed the remove-launcher.macosx-fragment branch from a4557ef to f3e4a20 Compare September 16, 2024 18:07

HannesWell force-pushed the remove-launcher.macosx-fragment branch from f3e4a20 to d7926bd Compare September 19, 2024 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove short-named equinox.launcher.cocoa.macosx fragment #622

Remove short-named equinox.launcher.cocoa.macosx fragment #622

HannesWell commented May 8, 2024

HannesWell commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

HannesWell commented May 9, 2024

HannesWell commented May 9, 2024

Phillipus commented May 11, 2024

Phillipus commented May 11, 2024 •

edited

Loading

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024 •

edited

Loading

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 13, 2024

Phillipus commented May 13, 2024

sravanlakkimsetti commented May 13, 2024

HannesWell commented May 13, 2024 •

edited

Loading

Phillipus commented May 13, 2024

HannesWell commented May 13, 2024

HannesWell commented Sep 19, 2024 •

edited

Loading

Remove short-named equinox.launcher.cocoa.macosx fragment #622

Are you sure you want to change the base?

Remove short-named equinox.launcher.cocoa.macosx fragment #622

Conversation

HannesWell commented May 8, 2024

HannesWell commented May 8, 2024 • edited Loading

github-actions bot commented May 8, 2024 • edited Loading

Test Results

HannesWell commented May 9, 2024

HannesWell commented May 9, 2024

Phillipus commented May 11, 2024

Phillipus commented May 11, 2024 • edited Loading

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024 • edited Loading

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

HannesWell commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 12, 2024

Phillipus commented May 13, 2024

Phillipus commented May 13, 2024

sravanlakkimsetti commented May 13, 2024

HannesWell commented May 13, 2024 • edited Loading

Phillipus commented May 13, 2024

HannesWell commented May 13, 2024

HannesWell commented Sep 19, 2024 • edited Loading

HannesWell commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading

Phillipus commented May 11, 2024 •

edited

Loading

Phillipus commented May 12, 2024 •

edited

Loading

HannesWell commented May 13, 2024 •

edited

Loading

HannesWell commented Sep 19, 2024 •

edited

Loading