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

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

Merged

Conversation

HannesWell
Copy link
Member

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 HannesWell requested a review from tjwatson May 8, 2024 22:04
@HannesWell
Copy link
Member Author

HannesWell commented May 8, 2024

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).

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

github-actions bot commented May 8, 2024

Test Results

  220 files   -   440    220 suites   - 440   26m 46s ⏱️ - 47m 23s
2 201 tests ±    0  2 154 ✅ ±    0  47 💤 ± 0  0 ❌ ±0 
2 249 runs   - 4 498  2 202 ✅  - 4 402  47 💤  - 96  0 ❌ ±0 

Results for commit d7926bd. ± Comparison against base commit cbd261e.

♻️ This comment has been updated with latest results.

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

@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.

HannesWell added a commit to eclipse-equinox/equinox.binaries that referenced this pull request May 9, 2024
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
Copy link
Member Author

@Phillipus
Copy link

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
Copy link

Phillipus commented May 11, 2024

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
Copy link
Member Author

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
Copy link

@HannesWell See #625 (comment)

@Phillipus
Copy link

Sorry, I just read this:

if this PR would fix #625?

Will check the PR tomorrow!

@Phillipus
Copy link

Phillipus commented May 12, 2024

@HannesWell Actually I just did a quick smoke test:

  1. Copied your Main code from your PR to my local copy
  2. 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
  3. Launched child RCP app - splash screen present and correct!

@HannesWell
Copy link
Member Author

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
Copy link

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

  1. Launch primary Eclipse or RCP app
  2. 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
Copy link

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
Copy link
Member Author

  1. 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
Copy link

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

Here:

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

arch is aarch64

@Phillipus
Copy link

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

@Phillipus
Copy link

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
Copy link

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
Copy link
Member

@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
Copy link
Member Author

HannesWell commented May 13, 2024

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

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.

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

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
Copy link
Member Author

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.

@HannesWell 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 added a commit to HannesWell/tycho that referenced this pull request May 19, 2024
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
HannesWell added a commit to HannesWell/p2 that referenced this pull request May 19, 2024
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
HannesWell added a commit to HannesWell/p2 that referenced this pull request May 19, 2024
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
HannesWell added a commit to eclipse-equinox/p2 that referenced this pull request May 19, 2024
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
HannesWell added a commit to HannesWell/tycho that referenced this pull request May 20, 2024
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
HannesWell added a commit to HannesWell/tycho that referenced this pull request May 20, 2024
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
HannesWell added a commit to eclipse-tycho/tycho that referenced this pull request May 20, 2024
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
HannesWell added a commit to HannesWell/tycho that referenced this pull request May 20, 2024
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
HannesWell added a commit to eclipse-tycho/tycho that referenced this pull request May 20, 2024
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
HannesWell added a commit to eclipse-equinox/equinox.binaries that referenced this pull request Jun 28, 2024
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 added a commit to eclipse-equinox/equinox.binaries that referenced this pull request Aug 14, 2024
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 HannesWell force-pushed the remove-launcher.macosx-fragment branch from 2d4ae1e to a4557ef Compare August 14, 2024 21:08
@HannesWell HannesWell force-pushed the remove-launcher.macosx-fragment branch 2 times, most recently from f3e4a20 to d7926bd Compare September 19, 2024 23:19
@HannesWell
Copy link
Member Author

HannesWell commented Sep 19, 2024

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

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 HannesWell force-pushed the remove-launcher.macosx-fragment branch from d7926bd to 5815854 Compare September 20, 2024 17:07
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 HannesWell force-pushed the remove-launcher.macosx-fragment branch from 5815854 to 590aebf Compare September 20, 2024 17:08
@HannesWell
Copy link
Member Author

Sporadic build failures are unrelated (eclipse-platform/eclipse.platform.releng.aggregator#2360 (comment)).

Submitting.

@HannesWell HannesWell merged commit 773fe39 into eclipse-equinox:master Sep 20, 2024
12 of 15 checks passed
@HannesWell HannesWell deleted the remove-launcher.macosx-fragment branch September 20, 2024 17:43
HannesWell added a commit to eclipse-equinox/equinox.binaries that referenced this pull request Sep 20, 2024
The fragment org.eclipse.equinox.launcher.cocoa.macosx is removed in [1]
since there are architecture specific replacements now.

[1] - eclipse-equinox/equinox#622
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