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

[ANT] Remove obsolete comments and update ant.launching/remote.jar #1664

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HannesWell
Copy link
Member

@HannesWell HannesWell commented Dec 17, 2024

Also remove obsolete comments.

Follow-up on

Currently the ANT remote.jar pops-up as changed in my workspace and I didn't find an immediate hint that it's built in the CI.
So I guess it has to be updated and committed manually?

Copy link
Contributor

github-actions bot commented Dec 17, 2024

Test Results

 1 755 files  ±0   1 755 suites  ±0   1h 31m 58s ⏱️ + 3m 49s
 4 170 tests ±0   4 148 ✅ ±0   22 💤 ±0  0 ❌ ±0 
13 107 runs  ±0  12 943 ✅ ±0  164 💤 ±0  0 ❌ ±0 

Results for commit ae08158. ± Comparison against base commit ca5ed8a.

♻️ This comment has been updated with latest results.

@HannesWell HannesWell changed the title [ANT] De-duplicat SecurityManager support check and update remote.jar [ANT] Remove obsolete comments and update ant.launching/remote.jar Dec 17, 2024
@HannesWell
Copy link
Member Author

The de-duplication didn't work because all the InternalAntRunner classes seem to go into separate jars.
Giving up on this.

@merks
Copy link
Contributor

merks commented Dec 18, 2024

The de-duplication didn't work because all the InternalAntRunner classes seem to go into separate jars. Giving up on this.

Yes, welcome to my world!

@merks
Copy link
Contributor

merks commented Dec 18, 2024

Hmm. I would have hoped the build would build them, but apparent not:

image

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

Sorry I should have reverted the public thing after reuse failed.

@vogella
Copy link
Contributor

vogella commented Dec 18, 2024

As the SDK requires now Java 21, maybe we could also update Ant to use Java 21 and remove the conditional check?

@laeubi
Copy link
Contributor

laeubi commented Dec 18, 2024

Only the help requires java 21

@vogella
Copy link
Contributor

vogella commented Dec 18, 2024

Only the help requires java 21

Yes and therefore the SDK requires Java 21 as it contains help. And we are open to move more components to Java 21, see recent PMC mail from @akurtakov.

@iloveeclipse
Copy link
Member

This would mean users that have Java < 21 configured to run ant files will be broken.

@vogella
Copy link
Contributor

vogella commented Dec 18, 2024

This would mean users that have Java < 21 configured to run ant files will be broken.

I think that is similar to user who would like to run the SDK with Java < 21. This is also not possible anymore.

Just to clarify: I don't feel strongly about updating Ant to Java 21 but from the discussion it sounded to me that supporting both is complex and time consuming so I wanted to add this option. As others are working on it I do not plan to mingle into their code changes.

@merks
Copy link
Contributor

merks commented Dec 18, 2024

Please let's not do unnecessary disruptive things! Note that some of these things run inside an actual ant task running inside a separate user-configured JVM. Note too that Ant itself still runs on Java 8:

image

@iloveeclipse
Copy link
Member

I think that is similar to user who would like to run the SDK with Java < 21. This is also not possible anymore.

It is not! You can use Eclipse running on Java 21 but that doesn't mean all your products are supposed to use Java 21 now! This is the basics of IDE development!

@HannesWell
Copy link
Member Author

Sorry I should have reverted the public thing after reuse failed.

Actually it was the other way round. I havn't removed it from my experiments but did it now.

Hmm. I would have hoped the build would build them, but apparent not:

As far as I can tell the current version of the remote.jar was compiled with 11.0.14+9 (Eclipse Adoptium) (at least according to the MANIFEST.MF.)
Therefore I wonder for how long this jar has not been recompiled and if just relying on the rebuild from the workspace is the right thing. I guess the class files now target Java-17? And before probably targeted Java-11.

Can someone more familiar with ant respectively this part can check this advice the right thing to do?

I think that is similar to user who would like to run the SDK with Java < 21. This is also not possible anymore.

It is not! You can use Eclipse running on Java 21 but that doesn't mean all your products are supposed to use Java 21 now!

Yes.

@iloveeclipse
Copy link
Member

I guess the class files now target Java-17? And before probably targeted Java-11.

I hope not, it should have been 1.7 or 1.8.
One can disassemble (javap or Bytecode Outline) class files to see to which JLS target they were compiled. JDK mentiomed in the Manifest doesn't mean anything as you can compile on Java 23 with target 1.8.

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.

5 participants