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

Restructure JDT core to get proper ecj bundle #182

Merged
merged 1 commit into from Dec 1, 2022

Conversation

iloveeclipse
Copy link
Member

@iloveeclipse iloveeclipse commented Jun 30, 2022

[DRAFT] An idea how to restructure JDT core to get proper ecj bundle

The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one should be renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and be a proper maven library.

It is actually the ecj compiler library without any dependencies, that
could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.
Unfortunately, there are two split packages:

org.eclipse.jdt.internal.compiler
org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they would be inside
org.eclipse.jdt.core.compiler.batch.

TODO:

  1. What I did NOT tried is to re-write all the magic scripts that build
    and package separated ecj library out of jdt.core.
  2. The 3 antadapter classes are now split over ecj and jdt core, because
    BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
    code.
  3. ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
    time 2017, it seem to be used for man pages.
  4. pom from jdt core will need an adoption.
  5. org.eclipse.jdt-feature need to be updated
  6. TBC

See

NOTE:

Since the patch moves subtree, "usual" rebase in egit doesn't work (and I guess in github too)

To properly rebase it on latest master, following command should be used:
git rebase -s subtree master

@guw
Copy link
Contributor

guw commented Aug 5, 2022

@iloveeclipse +1 for moving the fragments into the ECJ bundle. They offer additional functionality to make the ECJ better integrate with the Java ecosystem. Technically they are not required for the IDE. However, they do make ECJ a better offering with those APIs and should be shipped together. It doesn't make sense to have them be separate fragments.

My hypothesis is that they are separate because they have been created long time ago while JDT was much more strict with who and what goes into jdt.core.

@iloveeclipse
Copy link
Member Author

OK, got rebase working with git rebase -s subtree master (so git considered moved subtrees). At least in the IDE still no errors...

@laeubi
Copy link
Contributor

laeubi commented Oct 19, 2022

@iloveeclipse I have no idea why but the unit org.eclipse.jdt.core.compiler.batch is explicitly excluded from the target resolution here:

eclipse.jdt.core/pom.xml

Lines 97 to 101 in 53f5b7b

<filter>
<type>osgi-bundle</type>
<id>org.eclipse.jdt.core.compiler.batch</id>
<removeAll/>
</filter>

if I remove this the build can resolve ecj.core...

@iloveeclipse
Copy link
Member Author

This is probably because it was "artificially" generated out of jdt core and other bundles. OK, I will remove that later to see where the build will fail next.

@iloveeclipse iloveeclipse force-pushed the issue_181 branch 4 times, most recently from 80a80c2 to e8c3354 Compare October 20, 2022 12:09
@mpalat
Copy link
Contributor

mpalat commented Nov 17, 2022

Hey Andrey,
I know its a little late to comment/ask info, but neverthless - better late than never..comments/queries inline.

It is actually the ecj compiler library without any dependencies, that could be consumed by JDT and the rest of the world.
First of all, I believe this is indeed a great initiative to bring this structure to JDT.. Thank you!

It must be required and re-exported by JDT core. Unfortunately, there are two split packages:
Here, the query is what did you mean by required by JDT Core? I understand this as you meant that the ecj (the standalone part) to be required by Other Parts of JDT Core - like Search/Content-Assist/DOM etc Part of the JDT Core as we know now and then re-exported by this part. Is this understanding correct?

@iloveeclipse
Copy link
Member Author

@mpalat : sorry I haven't updated the patch here on last commit, below is the last state I've commented on #181.

The split package is solved by reexporting packages, and yes, your understanding is correct.

Update

With the help from @laeubi I was able to fix first round of maven errors.

Something was built, so that even (all gerrit?) tests were executed.

So far only 24 failures in org.eclipse.jdt.apt.pluggable.tests.

The same tests run fine in the IDE, and I don't see clear pointer why don't they run in maven.
If anyone has an idea, what is special about org.eclipse.jdt.apt.pluggable.tests, don't hesitate to leave a comment.

@laeubi
Copy link
Contributor

laeubi commented Nov 17, 2022

@stephan-herrmann can you help with the tests maybe?

I see in one test:

junit.framework.AssertionFailedError: Processor did not run

this seems that on the maven setup something is either missing (e.g. "the" Processor) or such an item has unresolved requirements and thus can't be loaded.

@iloveeclipse I don't know if there are maybe any resources relevant here, there is a stupid behaviour in PDE that it includes all resources in the IDE, but maintains a build.properties what resources are included when the bundle is build... so it might be that a resource that is required here is missing in the build-jar.

Beside that, maybe it could reveal some issues if one is not using jdt.core but the compiler bundle directly where appropriate? But thats just a wild guess...

<build>
<plugins>
<plugin>
<artifactId>maven-antrun-plugin</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

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

@akurtakov can you maybe help here so we replace this by maven resource filtering? seems more suitable than an ant-run ... maybe similar to

https://github.com/eclipse-tycho/tycho/blob/master/tycho-core/pom.xml#L30

Copy link
Contributor

Choose a reason for hiding this comment

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

It's in my todo list to look at this one at some point.

@iloveeclipse
Copy link
Member Author

I believe the problem with org.eclipse.jdt.apt.pluggable.tests could be that the org.eclipse.jdt.apt.pluggable.core imported wrong packages from old apt fragments of jdt core because the packages merged into batch compiler were not re-exported to jdt.core.
I will push a patch in a moment to see if tuning dependencies a bit helps.

@iloveeclipse
Copy link
Member Author

Hmm, now it says

junit.framework.AssertionFailedError: apt.pluggable.tests plug-in was not found in project factory path
	at junit.framework.Assert.fail(Assert.java:57)
	at junit.framework.Assert.assertTrue(Assert.java:22)
	at junit.framework.TestCase.assertTrue(TestCase.java:192)
	at org.eclipse.jdt.apt.pluggable.tests.InfrastructureTests.testFactoryPathContents(InfrastructureTests.java:91)

and I can reproduce in the IDE if I exclude compiler.apt fragment from the target platform.

Same with test below

expected:<NO[ ERRORS]> but was:<NO[T RUN]>
	at org.eclipse.jdt.core.tests.junit.extension.TestCase.assertStringEquals(TestCase.java:266)
	at org.eclipse.jdt.core.tests.junit.extension.TestCase.assertEquals(TestCase.java:242)
	at org.eclipse.jdt.apt.pluggable.tests.FilerTests.testNoCollisionInSameModule(FilerTests.java:453)

So that thing somehow can't find re-exported packages at runtime. batch plugin re-exports them to jdt core and it is fine for compilation, but that seem to be not enough at runtime.

@stephan-herrmann
Copy link
Contributor

junit.framework.AssertionFailedError: apt.pluggable.tests plug-in was not found in project factory path

Have a look at org.eclipse.jdt.apt.core.internal.AptPlugin.canRunJava6Processors(): this line no longer works:

		return Platform.getBundle("org.eclipse.jdt.compiler.apt") != null; //$NON-NLS-1$

Perhaps it can just be removed for good, given that we no longer support ecj on Java < 6.

@iloveeclipse
Copy link
Member Author

Have a look at org.eclipse.jdt.apt.core.internal.AptPlugin.canRunJava6Processors()

Yep, that was it.

@iloveeclipse
Copy link
Member Author

iloveeclipse commented Nov 17, 2022

Cool, no test fails, let merge for RC1 !!! :-)

Just a note for those who care: "git blame" still works for files in batch compiler, the moved files are understood as "moved" by git so the entire history is still there (at least in latest EGit).

Now the boring part starts:

  • understand which non-Java resources from JDT.core should be also copied or moved into batch compiler
  • polish probably poms and check that we produce "same" bundles
  • understand which ant, maven and script build steps are not needed / must be adopted in JDT/SDK area for the new real bundle.
  • check if the SDK resulted from that can be used as IDE
  • check if the ECJ resulted from that can be used on command line / ant / maven / tycho compilation
  • TBC, whatever else.

@iloveeclipse
Copy link
Member Author

image

@iloveeclipse iloveeclipse changed the title [DRAFT] An idea how to restructure JDT core to get proper ecj bundle Restructure JDT core to get proper ecj bundle Dec 1, 2022
@iloveeclipse iloveeclipse marked this pull request as ready for review December 1, 2022 14:46
@iloveeclipse
Copy link
Member Author

@ALL : if this state builds & tests are OK would it make sense to merge and "break" SDK build?

This would at least make sure everyone else interested to get things done in SDK would need to help here.

@akurtakov
Copy link
Contributor

Yes, please. Now is the perfect time for that work.

The org.eclipse.jdt.core.ecj.validation was a "dummy" bundle so far,
used only to validate compilation issues in IDE.

That one is renamed (org.eclipse.jdt.core.ecj.validation ->
org.eclipse.jdt.core.compiler.batch) and is a proper maven library now.

It is actually the ecj compiler library without any dependencies (except
optional ant), that could be consumed by JDT and the rest of the world.

It must be required and re-exported by JDT core.

Unfortunately, there are two split packages:

- org.eclipse.jdt.internal.compiler
- org.eclipse.jdt.internal.compiler.parser

So the new bundle exports them to jdt.core and jdt.core re-exports.

org.eclipse.jdt.compiler.apt and org.eclipse.jdt.compiler.tool were
fragments of jdt.core, now they are inside
org.eclipse.jdt.core.compiler.batch.

TODO:

1) What I did NOT tried is to re-write all the magic scripts that build
and package separated ecj library out of jdt.core.
2) The 3 antadapter classes are now split over ecj and jdt core, because
BuildJarIndex.java and CheckDebugAttributes.java depend on JDT core
code.
3) ecj.1 and build_ecj.xml aren't touched yet. ecj.1 was touched last
time 2017, it seem to be used for man pages.
4) pom from jdt core will need an adoption.
5) org.eclipse.jdt-feature need to be updated
6) TBC

See
- eclipse-jdt#181
- eclipse-platform/eclipse.platform.ua#18
@iloveeclipse
Copy link
Member Author

OK, the build is green, I will merge that now, with ~100% probability to break SDK build.
I would like to trigger SDK build, but I didn't find it anymore reading https://github.com/eclipse-platform/eclipse.platform.releng.aggregator and looking at https://ci.eclipse.org/releng/view/Builds/

@iloveeclipse iloveeclipse merged commit 457e222 into eclipse-jdt:master Dec 1, 2022
@iloveeclipse iloveeclipse deleted the issue_181 branch December 1, 2022 15:49
@laeubi
Copy link
Contributor

laeubi commented Dec 1, 2022

@iloveeclipse why do you think this will break SDK build (and what needs to be done/changed to mitigate this?).

@iloveeclipse
Copy link
Member Author

Because batch compiler / ecj jar had extra treatment and I'm pretty sure some of "extra" steps missing we don't run in Jenkins to "manually merge & copy & publish not-really-maven library to some SDK special places".

E.g. there is a dedicated section for JDT Core Batch Compiler & source jars on download page: https://download.eclipse.org/eclipse/downloads/drops4/I20221201-0500/

I bet this will not work, as I have no idea where the final jars were produced and if the existing code finds them now in maven target directory.

@iloveeclipse
Copy link
Member Author

@akurtakov : I must be blind, I can't find where 4.27 build jobs are, I don't see them here: https://ci.eclipse.org/releng/view/Builds/
image

@laeubi
Copy link
Contributor

laeubi commented Dec 1, 2022

@iloveeclipse
Copy link
Member Author

Arrgh. That's somehow hidden. I will trigger one now.

@iloveeclipse
Copy link
Member Author

I've added links to readme: eclipse-platform/eclipse.platform.releng.aggregator#733

@akurtakov
Copy link
Contributor

The jobs are autocreated from git content in https://ci.eclipse.org/releng/job/Builds/ and https://ci.eclipse.org/releng/job/Builds/ . Real structure instead of jenkins view.

@stephan-herrmann
Copy link
Contributor

First breakage can be fixed well inside JDT: https://github.com/eclipse-jdt/eclipse.jdt/blob/master/org.eclipse.jdt-feature/feature.xml

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.

None yet

6 participants