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 nolonger supported converterJclMin #3332

Merged

Conversation

akurtakov
Copy link
Contributor

Versions older than 1.8 are not supported and thus shouldn't be needed nor referenced anymore.

@akurtakov
Copy link
Contributor Author

akurtakov commented Nov 20, 2024

This looks like continuation of #3227 and I start with it as I figured converterJclMin1.8 lacks NoSuchFieldError while trying to enable ASTConververt15JLS8Test from #2758

@akurtakov akurtakov force-pushed the removeoldconverterjclmin branch from 4f3ce60 to 006678d Compare November 20, 2024 18:35
@akurtakov akurtakov self-assigned this Nov 20, 2024
@akurtakov
Copy link
Contributor Author

@iloveeclipse @stephan-herrmann Do you see some fundamental reason to keep these? If not, I'll work on fixing the tests.

@akurtakov akurtakov changed the title Remove nolonger support converterJclMin Remove nolonger supported converterJclMin Nov 20, 2024
@iloveeclipse
Copy link
Member

I don't, as long as tests are green, but only because I have no idea how it was designed. It just happened to me to stumble over the various jclMinXYZ jars which someone somehow built and the parallel exisring jclConverterXYZ jars.

@akurtakov akurtakov force-pushed the removeoldconverterjclmin branch 2 times, most recently from c2e7771 to 9f68b58 Compare November 20, 2024 20:36
@stephan-herrmann
Copy link
Contributor

This looks like continuation of #3227 and I start with it as I figured converterJclMin1.8 lacks NoSuchFieldError while trying to enable ASTConververt15JLS8Test from #2758

Is removing old jars a prerequisite or otherwise related to fixing the test?

Do you see some fundamental reason to keep these? If not, I'll work on fixing the tests.

Nothing fundamental, I just suggest to look for a path that doesn't force changes in too many tests.

I have no idea how it was designed. It just happened to me to stumble over the various jclMinXYZ jars which someone somehow built and the parallel exisring jclConverterXYZ jars.

I cannot comment on the original motivation, but I can see that

  • jars have different sizes, large to small: jclFull, converterJcl, jclMin
  • small class libraries help keeping test execution time low
  • class libraries that we control ensure that results don't unnecessarily change (see also my change of strategy in #3227).

... jars which someone somehow built ...

building is no longer a mystery since #2727 is it?

@akurtakov
Copy link
Contributor Author

akurtakov commented Nov 21, 2024

This looks like continuation of #3227 and I start with it as I figured converterJclMin1.8 lacks NoSuchFieldError while trying to enable ASTConververt15JLS8Test from #2758

Is removing old jars a prerequisite or otherwise related to fixing the test?

It is actually. As can be seen at

<antcall target="converterJclMin">
building converterJCLMin1.8 is commented out so changes to it have no effect.
And trying to use this build.xml file doesn't complete successfully with:

jclMin_notModular:
    [javac] /home/akurtakov/git/eclipse.jdt.core/org.eclipse.jdt.core.tests.model/JCL/build.xml:148: warning: 'includeantruntime' was not set, defaulting to build.sysclasspath=last; set to false for repeatable builds
    [javac] Compiling 13 source files to /home/akurtakov/git/eclipse.jdt.core/JCL/jclMin1.8/bin
    [javac] /home/akurtakov/git/eclipse.jdt.core/JCL/jclMin1.8/src/java/lang/annotation/ElementType.java:2: error: cannot find symbol
    [javac] public enum ElementType {
    [javac]        ^
    [javac]   symbol:   method valueOf(Class,String)
    [javac]   location: class Enum<T>
    [javac]   where T is a type-variable:
    [javac]     T extends Enum<T> declared in class Enum
    [javac] 1 error
    [javac] Fatal Error: Unable to find method valueOf

BUILD FAILED
/home/akurtakov/git/eclipse.jdt.core/org.eclipse.jdt.core.tests.model/JCL/build.xml:94: The following error occurred while executing this line:
/home/akurtakov/git/eclipse.jdt.core/org.eclipse.jdt.core.tests.model/JCL/build.xml:148: Compile failed; see the compiler error output for details.

which is the next thing I plan to look at after this one and continuation to it no longer commenting 1.8 rebuild.

@stephan-herrmann
Copy link
Contributor

@akurtakov I very much appreciate your efforts to fix ASTConververt15JLS8Test! I just wonder how we can avoid this to expand into a large moving target effort with limited benefit.

Regarding jclMin: the build has successfully run just recently (on my machine :) ). Have you seen https://www.eclipse.org/lists/jdt-dev/msg02453.html - not seeing any response there nor in #2727 I assumed everybody is happy with the status. If that is not the case please wait until next week when I will be able to help, to avoid unnecessary back and forth.

When I worked on #2727 actually my goal was to facilitate working on new / recent Java versions. Once #2758 is resolved I hope we will NEVER have to build any of the old version JCLs again. Those are intended to provide stability. No further changes intended.

So perhaps building all those jars always is actually a inappropriate approach. Possible steps:

  • remove the default target from build.xml so we'll build only those jars where changes are definitely needed
  • remove build.xml altogether and replace with a simple *.jardesc in each of the JCL projects

And, yes, building classes in java.lang et al is always a tricky issue, as the compiler assumes that these classes already exist before compilation starts, and ecj and javac seem to make different such assumptions. That's why an ant build using javac can indeed fail, despite ecj having successfully compiled everything.

@akurtakov
Copy link
Contributor Author

@akurtakov I very much appreciate your efforts to fix ASTConververt15JLS8Test! I just wonder how we can avoid this to expand into a large moving target effort with limited benefit.

If it makes maintenance clearer for someone not involved daily (like me) it's worth it and I consider it time well spend.

Regarding jclMin: the build has successfully run just recently (on my machine :) ). Have you seen https://www.eclipse.org/lists/jdt-dev/msg02453.html - not seeing any response there nor in #2727 I assumed everybody is happy with the status. If that is not the case please wait until next week when I will be able to help, to avoid unnecessary back and forth.

I have to admit I haven't paid much attention to these changes and having others deal with it.

When I worked on #2727 actually my goal was to facilitate working on new / recent Java versions. Once #2758 is resolved I hope we will NEVER have to build any of the old version JCLs again. Those are intended to provide stability. No further changes intended.

Changes will have to happen when jdt min supported version moves up as tests on old versions will need updates.

So perhaps building all those jars always is actually a inappropriate approach.

I fully agree here, rebuilding when there is need should be possible though (like in this case).

@stephan-herrmann
Copy link
Contributor

Changes will have to happen when jdt min supported version moves up as tests on old versions will need updates.

Once the current set of changes is done, all 1.8 jars should have reached their final state, as there will no longer be any tests at lower levels that will have to migrate to 1.8 😄

Additionally, I might do a general scan if any JCL X lacks feature present in JCL Y for Y<X, to avoid any such need to touch tests or JCL jars when any version goes EOL.

@akurtakov
Copy link
Contributor Author

Changes will have to happen when jdt min supported version moves up as tests on old versions will need updates.

Once the current set of changes is done, all 1.8 jars should have reached their final state, as there will no longer be any tests at lower levels that will have to migrate to 1.8 😄

Additionally, I might do a general scan if any JCL X lacks feature present in JCL Y for Y<X, to avoid any such need to touch tests or JCL jars when any version goes EOL.

That would be very nice and would definetely minimize the changes needed whenever jdt.core moves to e.g. Java 11 as a min version.

Versions older than 1.8 are not supported and thus shouldn't be needed
anymore.
@akurtakov akurtakov force-pushed the removeoldconverterjclmin branch from 9f68b58 to 25daea0 Compare November 26, 2024 12:06
@akurtakov
Copy link
Contributor Author

Is there a reason not to merge this one in?

@stephan-herrmann
Copy link
Contributor

Is there a reason not to merge this one in?

no :)

@stephan-herrmann stephan-herrmann merged commit 7ad8448 into eclipse-jdt:master Nov 26, 2024
10 checks passed
jarthana added a commit that referenced this pull request Dec 6, 2024
* Remove nolonger support converterJclMin (#3332)

Versions older than 1.8 are not supported and thus shouldn't be needed
anymore.

* Fix and enable Java50Tests#testMissingRequiredBinaries

* Version bump(s) for 4.35 stream

* Test failures in I-Builds due to less diagnostics being emitted (#3357)

* Fixes #3356

* Textual problem indicator goes wild with lamda (#3358)

* jclMin23: ignore missing serializable problem

"The serializable class Long does not declare a static final
serialVersionUID field of type"

* Fix FieldLocator and MethodLocator to support local/anonymous classes (#3314)

- Fix FieldLocator.reportDeclaration() and
  MethodLocator.reportDeclaration() to find the anonymous or local
  type for the declaration rather than to return
- add new tests to JavaSearchBugsTests
- fixes #3308

* Codegen Primitives in record comonent patterns to be enabled with null check before calling accessor(#3361)

Before generating an invoke of accessor of a record component, do a null check if primitive conversions are involved.

* java.lang.StackOverflowError during "Requesting Java AST from selection" (#3373)

+ resilience: avoid accepting a sourceType being completed already
+ better hiding of modules seen via the classpath

Fixes #3273

* Add NoSuchFieldError to converterJclMin18 (#3368)

Add NoSuchFieldError to converterJclMin18
 + build the jar
 + avoid new warning

Enable couple of tests fixed by this.

---------
Co-authored-by: Stephan Herrmann <stephan.herrmann@berlin.de>

* Incorrect control flow analysis causes statement subsequent to a switch statement to be flagged unreachable under some circumstances(#3377)

* Fixes #3376

* Remove unused api problem filter

* Update tycho build to 4.0.10

* Completion for unimported types doesn't work.

- use MissingTypesGuesser to select all missing types and offer their completion
- Fixes #1502

* [Enhanced Switch] Wrong error message: Cannot switch on a value of type Integer... at levels that don't support enhanced switch (#3380)

* Fixes #3379

---------

Co-authored-by: Александър Куртаков <akurtakov@gmail.com>
Co-authored-by: Eclipse JDT Bot <jdt-bot@eclipse.org>
Co-authored-by: Srikanth Sankaran <131454720+srikanth-sankaran@users.noreply.github.com>
Co-authored-by: Jörg Kubitz <jkubitz-eclipse@gmx.de>
Co-authored-by: Jeff Johnston <jjohnstn@redhat.com>
Co-authored-by: Manoj  N Palat <manoj.palat@in.ibm.com>
Co-authored-by: Stephan Herrmann <stephan.herrmann@berlin.de>
Co-authored-by: Snjeza <snjezana.peco@redhat.com>
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