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

Comparator errors in I20241106-1800 #1461

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Conversation

jarthana
Copy link
Contributor

@jarthana jarthana commented Nov 7, 2024

These are expected changes due to the compiler changes via eclipse-jdt/eclipse.jdt.core#3254

These are expected changes due to the compiler changes via eclipse-jdt/eclipse.jdt.core#3254
@jarthana
Copy link
Contributor Author

jarthana commented Nov 7, 2024

Copying @srikanth-sankaran

@srikanth-sankaran
Copy link

I will confirm shortly. At first blush nothing looks amiss. Compiler has stopped using secret variables for yielding the value of a switch expression and directly leaves it on the operandi stack. That accounts for stack size changed and variable index changes in the byte code.
Ri
I'll confirm once analysis finishes.

Who can follow up with a bundle update ? I don't have the commit rights.

@jarthana
Copy link
Contributor Author

jarthana commented Nov 7, 2024

Not sure @iloveeclipse is back yet. I guess either @akurtakov or @HannesWell can help.

@akurtakov
Copy link
Member

Just say here that it's as expected and should go in and whoever is around will push it.

Copy link

github-actions bot commented Nov 7, 2024

Test Results

   285 files  +  6     285 suites  +6   49m 9s ⏱️ +40s
 3 586 tests ±  0   3 510 ✅ +  2   76 💤  -  1  0 ❌  - 1 
10 950 runs  +194  10 719 ✅ +167  231 💤 +28  0 ❌  - 1 

Results for commit 40969fb. ± Comparison against base commit f3ba023.

@srikanth-sankaran
Copy link

Just say here that it's as expected and should go in and whoever is around will push it.

I have studied these :

1.  eclipse.pde/apitools/org.eclipse.pde.api.tools/.polyglot.META-INF
   no-classifier: different
      org/eclipse/pde/api/tools/internal/builder/IncrementalApiBuilder$ResourceDeltaVisitor.class: different
      org/eclipse/pde/api/tools/internal/util/Signatures.class: different
    The main artifact has been replaced with the baseline version.
    The following attached artifacts have been replaced with the baseline version: [sources]

No harm, no foul.

As alluded to above, the unnecessary store into and load from secret location used for yielded values in a switch expression go away and this changes byte code offsets and sometimes bytecode size/shape too.

So we can proceed with the version bump.

Thanks for your help with this, gentlemen.

@srikanth-sankaran
Copy link

Test Results

   285 files  +  6     285 suites  +6   49m 9s ⏱️ +40s  3 586 tests ±  0   3 510 ✅ +  2   76 💤  -  1  0 ❌  - 1  10 950 runs  +194  10 719 ✅ +167  231 💤 +28  0 ❌  - 1 

Results for commit 40969fb. ± Comparison against base commit f3ba023.

I have no idea how to interpret this, but overall https://download.eclipse.org/eclipse/downloads/drops4/I20241106-1800/testResults.php looks good with pde entirely clean.

@akurtakov akurtakov merged commit ee1f4a3 into eclipse-pde:master Nov 7, 2024
18 checks passed
@akurtakov
Copy link
Member

New I-build started https://ci.eclipse.org/releng/job/Builds/job/I-build-4.34/122/

@laeubi
Copy link
Contributor

laeubi commented Nov 10, 2024

So we can proceed with the version bump.

We need a new compiler deployment as otherwise now PDE verification builds fail.

@srikanth-sankaran
Copy link

So we can proceed with the version bump.

We need a new compiler deployment as otherwise now PDE verification builds fail.

Sorry to ask a dumb question: what exactly if anything needs to happen from JDT Core side for this ??

@HannesWell
Copy link
Member

So we can proceed with the version bump.

We need a new compiler deployment as otherwise now PDE verification builds fail.

Sorry to ask a dumb question: what exactly if anything needs to happen from JDT Core side for this ??

I assume you have to run the JDT Jenkins job copyAndDeployJDTCompiler:
https://ci.eclipse.org/jdt/job/copyAndDeployJDTCompiler/

Probably with the parameters equals to
https://ci.eclipse.org/jdt/job/copyAndDeployJDTCompiler/177/parameters/
except that buildid should probably be I20241110-0600 and ecjversion should be 3.40.0.v20241110-0909.

The versions are the latest ones, maybe a bit earlier would also fit, but it should be at least the version that contains the change causing this.
In #1467 I'm trying out the latest snapshot.

@srikanth-sankaran
Copy link

@jarthana - FYI - Are you able to help with this ? TIA

@HannesWell
Copy link
Member

@jarthana or @srikanth-sankaran any updates on a new deployment?

@jarthana
Copy link
Contributor Author

Sorry, missed this one. I just tried with build I20241111-1800 but looks like all nodes are down. Let me follow up that.

@jarthana
Copy link
Contributor Author

The nodes have been brought up again and the maintenance work on repos is over. Now, the job is finally complete (I used build I20241111-1800, though).

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