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

[23] ECJ rejects 'module' as top level package in an import statement #2610

Merged

Conversation

stephan-herrmann
Copy link
Contributor

fixes #2607

@stephan-herrmann
Copy link
Contributor Author

Build failed with

22:11:06  [ERROR] Failed to execute goal org.eclipse.tycho.extras:tycho-p2-extras-plugin:4.0.8:compare-version-with-baselines (compare-attached-artifacts-with-release) on project org.eclipse.jdt.core.tests.builder: Baseline and reactor have the same fully qualified version, but different content
22:11:06  [ERROR] different
22:11:06  [ERROR]    META-INF/ECLIPSE_.RSA: present in baseline only
22:11:06  [ERROR]    META-INF/ECLIPSE_.SF: present in baseline only
22:11:06  [ERROR]    workspace/Test571363.jar: different
22:11:06  [ERROR]       META-INF/ECLIPSE_.RSA: present in baseline only
22:11:06  [ERROR]       META-INF/ECLIPSE_.SF: present in baseline only

However, org.eclipse.jdt.core.tests.builder was not touched, so why the different content??

@stephan-herrmann
Copy link
Contributor Author

stephan-herrmann commented Jun 19, 2024

That failure is persistent (see previous comment).

@jarthana I'm sensing a dejavu of having discusses this just recently, but can't remember any details (I'm getting old).

Master has bumped tests.builder to 3.12.500 on 2024-06-12. Do we need this merged into the beta branch for builds to succeed?

@iloveeclipse do we know, against which baseline the beta branch is compared? Is it something like the latest I-build on master?

@stephan-herrmann
Copy link
Contributor Author

Error message is the same as in eclipse-tycho/tycho#1053 which was said not to be a tycho issue, but an issue of building a version that cannot be matched in the baseline, IIUC.

@jarthana
Copy link
Member

jarthana commented Jun 20, 2024

@jarthana I'm sensing a dejavu of having discusses this just recently, but can't remember any details (I'm getting old).

Master has bumped tests.builder to 3.12.500 on 2024-06-12. Do we need this merged into the beta branch for builds to succeed?

I have done a merge now. Let me do a rebase of this PR. I see a rebase force-push this. Hope you are OK with a force-push in this scenario :)

@stephan-herrmann
Copy link
Contributor Author

@jarthana I'm sensing a dejavu of having discusses this just recently, but can't remember any details (I'm getting old).
Master has bumped tests.builder to 3.12.500 on 2024-06-12. Do we need this merged into the beta branch for builds to succeed?

I have done a merge now. Let me do a rebase of this PR. I see a rebase force-push this. Hope you are OK with a force-push in this scenario :)

Done :)

None of my objections against force-push are at play here (no ongoing review, no references to commits that are destroyed by the force-push, no risk of mixing rebase with unrelated code changes ...)

@jarthana
Copy link
Member

Looks like the merge didn't go well. I am looking into the tests failures. Will release a fix shortly and rebase this again. Sorry for the trouble.

@stephan-herrmann
Copy link
Contributor Author

Looks like the merge didn't go well. I am looking into the tests failures. Will release a fix shortly and rebase this again. Sorry for the trouble.

Thanks for looking into it.

@jarthana
Copy link
Member

OK, turns out the merge was alright. It's just that some of the changes for primitive pattern doesn't go well with some commits from master, notably 74f90bf. I will take help from @srikanth-sankaran and @mpalat to resolve this.

@mpalat
Copy link
Contributor

mpalat commented Jun 24, 2024

@jarthana there are API issues in merge -I get around 14 errors due to API assuming a 4.32 baseline. One is on ModuleRead.

@mpalat
Copy link
Contributor

mpalat commented Jun 24, 2024

OK, turns out the merge was alright. It's just that some of the changes for primitive pattern doesn't go well with some commits from master, notably 74f90bf. I will take help from @srikanth-sankaran and @mpalat to resolve this.

I will take a look at the PrimitiveTest failures.

@srikanth-sankaran
Copy link
Contributor

OK, turns out the merge was alright. It's just that some of the changes for primitive pattern doesn't go well with some commits from master, notably 74f90bf. I will take help from @srikanth-sankaran and @mpalat to resolve this.

@mpalat called to ask for some clarifications. Armed with them, he is investigating the failures seen in primitive type patterns. FYU

@mpalat
Copy link
Contributor

mpalat commented Jun 25, 2024

I have created #2633 for ASTRewrite failure which are unrelated to codegen

@mpalat mpalat mentioned this pull request Jun 25, 2024
3 tasks
@stephan-herrmann
Copy link
Contributor Author

I still (here and elsewhere) from SuperAfterStatementsTest:

	at org.eclipse.test.internal.performance.PerformanceMeterFactory.assertUniqueScenario(PerformanceMeterFactory.java:46)

assumably caused by this in TestAll:

	 since_22.add(SuperAfterStatementsTest.class);
	 since_22.add(SwitchPatternTest21.class);

	 ArrayList since_23 = new ArrayList();
	 since_23.add(SuperAfterStatementsTest.class);

which looks like at 23 we are simply executing the same test class twice.

I'm puzzled because I seem to remember that this had been fixed recently, no?

@mpalat
Copy link
Contributor

mpalat commented Jun 27, 2024

which looks like at 23 we are simply executing the same test class twice.

Thanks @stephan-herrmann for the analysis - was off yesterday unexpectedly and hence had a pause for a day - expecting to fix all the failures with this issue itself

@sravanlakkimsetti
Copy link
Member

 [ERROR]    META-INF/ECLIPSE_.RSA: present in baseline only
 [ERROR]    META-INF/ECLIPSE_.SF: present in baseline only
 [ERROR] -> [Help 1]

This error is coming from comparator. we were using tycho-p2:p2-metadata for comparator. The filter for signature files was set at https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/master/eclipse-platform-parent/pom.xml#L551-L554

In this case we got the error from tycho-p2-extras:compare-version-with-baselines

We will need to add similar filter in the configuration of tycho-p2-extras:compare-version-with-baselines. Probably need a change in https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/master/eclipse-platform-parent/pom.xml#L379

@sravanlakkimsetti
Copy link
Member

For now please add the following plugin configuration after

</plugin>

		<plugin>
	    	<groupId>org.eclipse.tycho.extras</groupId>
	    	<artifactId>tycho-p2-extras-plugin</artifactId>
			<version>${tycho.version}</version>
			<executions>
	        	<execution> <!-- Checks versions are properly bumped from one stream to the other -->
	           		<id>compare-attached-artifacts-with-release</id>
	           		<goals>
	             		<goal>compare-version-with-baselines</goal>
	           		</goals>
	           		<configuration>
	             		<skip>${compare-version-with-baselines.skip}</skip>
	             		<baselines>
	               			<baseline>${previous-release.baseline}</baseline>
	             		</baselines>
	             		<comparator>zip</comparator>
       		            <ignoredPatterns>
							<pattern>META-INF/ECLIPSE_.RSA</pattern>
							<pattern>META-INF/ECLIPSE_.SF</pattern>
            			</ignoredPatterns>
	           		</configuration>
	         	</execution>
	       </executions>
	     </plugin>

@mpalat
Copy link
Contributor

mpalat commented Jun 28, 2024

For now please add the following plugin configuration after
...

Thanks @sravanlakkimsetti - this worked! Finally a solution at the end of the saga!

@MohananRahul It maybe a good idea to record this solution in a troubleshooting guide for our own purpose.

@sravanlakkimsetti
Copy link
Member

For now please add the following plugin configuration after

</plugin>

		<plugin>
	    	<groupId>org.eclipse.tycho.extras</groupId>
	    	<artifactId>tycho-p2-extras-plugin</artifactId>
			<version>${tycho.version}</version>
			<executions>
	        	<execution> <!-- Checks versions are properly bumped from one stream to the other -->
	           		<id>compare-attached-artifacts-with-release</id>
	           		<goals>
	             		<goal>compare-version-with-baselines</goal>
	           		</goals>
	           		<configuration>
	             		<skip>${compare-version-with-baselines.skip}</skip>
	             		<baselines>
	               			<baseline>${previous-release.baseline}</baseline>
	             		</baselines>
	             		<comparator>zip</comparator>
       		            <ignoredPatterns>
							<pattern>META-INF/ECLIPSE_.RSA</pattern>
							<pattern>META-INF/ECLIPSE_.SF</pattern>
            			</ignoredPatterns>
	           		</configuration>
	         	</execution>
	       </executions>
	     </plugin>

This should not be required. We do have 2 comparator checks

  1. tycho-p2:p2-metadata
  2. tycho-p2-extras:compare-version-with-baselines

The first one already had the ignoredPatterns so the newly built bundle should have been replaced with the baseline. But in this case We are seeing

 [INFO] --- tycho-p2:4.0.8:p2-metadata-default (default-p2-metadata-default) @ org.eclipse.jdt.apt.pluggable.core ---
 [INFO] No baseline version org.eclipse.jdt:org.eclipse.jdt.apt.pluggable.core:eclipse-plugin:1.4.400-SNAPSHOT

This clearly indicating p2-metadata plugin is not able to find the baseline version though it present in the baseline see org.eclipse.jdt.apt.pluggable.core_1.4.400.v20240321-1252.jar

This is definitely a bug in tycho with p2-metadata

@stephan-herrmann
Copy link
Contributor Author

Can someone help me understand if BETA_JAVA23 is in any consistent state to get this bug fix merged?

If that branch is generally ill at health, perhaps we should create (should have) a separate issue for fundamental branch work??

@iloveeclipse
Copy link
Member

Can someone help me understand if BETA_JAVA23 is in any consistent state to get this bug fix merged?

Not sure which problem you have, but your PR is behind 1b14b69.

Try to rebase on latest state, and if the problem is still there, comment what exact problem you see.

@stephan-herrmann
Copy link
Contributor Author

stephan-herrmann commented Jul 2, 2024

Not sure which problem you have,

This PR is swamped with pages of comments relating to various build failures, none of which is related to my change, but each of the issues has block this PR. That's my problem :)

@iloveeclipse
Copy link
Member

image

Wow, you are really desperate :-)

@stephan-herrmann
Copy link
Contributor Author

Can someone help me understand if BETA_JAVA23 is in any consistent state to get this bug fix merged?

Not sure which problem you have, but your PR is behind 1b14b69.

Right, and that commit was merged via #2648 which itself had several problems

@stephan-herrmann
Copy link
Contributor Author

image

Wow, you are really desperate :-)

Were you hoping for another fight after your suggestion to rebase :)

I try to be pragmatic at times :)

And like I said above:

None of my objections against force-push are at play here

@jarthana
Copy link
Member

jarthana commented Jul 2, 2024

Can someone help me understand if BETA_JAVA23 is in any consistent state to get this bug fix merged?

If that branch is generally ill at health, perhaps we should create (should have) a separate issue for fundamental branch work??

Perhaps @mpalat knows better. I see the failure gone and just one API warning left, which I remember seeing in master as well.

@stephan-herrmann
Copy link
Contributor Author

Failing checks moved to #2657

@stephan-herrmann stephan-herrmann merged commit 52f1c7e into eclipse-jdt:BETA_JAVA23 Jul 2, 2024
3 of 6 checks passed
@stephan-herrmann stephan-herrmann deleted the issue2607 branch July 2, 2024 18:56
@stephan-herrmann stephan-herrmann added this to the BETA_JAVA23 milestone Jul 16, 2024
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.

6 participants