Sort filename (if given) by name for consistent output #1922

laeubi · 2024-01-30T08:08:30Z

Currently the generation of lamdas can change depending on when the source file is processed.

What it does

This sorts the array of files (if given) to always have a predictable order independent of order given on the commandline / arguments.

Relates to #1921

How to test

Use this project:

https://github.com/eclipse-pde/eclipse.pde/tree/master/ui/org.eclipse.pde.launching
pass the source files in different order to the batch compiler
see that depending on the order you get different lamda namings for the same code
with this patch the ordering of source file arguments do not matter

See #1921 for detailed compiler arguments, logs and example class files showing the difference.

Author checklist

I have tested my changes in a debugger with a Tycho build by manipulating the passed source files order
The change is following the coding conventions
I have signed the Eclipse Contributor Agreement (ECA)

iloveeclipse · 2024-01-30T15:48:18Z

Interesting, there are 34 test fails, some seem to be interesting like BatchCompilerTest

junit.framework.AssertionFailedError: missing file: /tmp/comptest/run.1706602408180/regression/bin1/X.class
	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.core.tests.compiler.regression.BatchCompilerTest.test097_per_source_output_directory(BatchCompilerTest.java:4537)

Some seem to rely on the input / error reporting order.

laeubi · 2024-01-30T15:52:02Z

Yes the test seem to expect a certain order but I can't judge if/how test to be adjusted.

I applied a workaround to Tycho to simulate what this patch does and all tests (including aggregator build) succeed and order was always random so I'm quite confident it should be a valid one...

iloveeclipse

The problem is "per output destination" feature, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=146554

Main.filenames & Main.destinationPaths should be in-sync, so change must be done not here but in configure()

iloveeclipse · 2024-01-30T16:25:52Z

I applied a workaround to Tycho to simulate what this patch does

I would keep that in Tycho, as the patch here is incomplete and needs to be fixed.

laeubi · 2024-01-31T07:05:03Z

The problem is "per output destination" feature, see https://bugs.eclipse.org/bugs/show_bug.cgi?id=146554

Main.filenames & Main.destinationPaths should be in-sync, so change must be done not here but in configure()

You are right, I also noticed that there are even public fields in this class so now I only sort the index here... another approach would be to do the ordering in the compiler itself I'll take a look at this as well.

laeubi · 2024-01-31T08:32:55Z

This now seems to pass the testsuite and already shows some classes that are different in JDT, I have createad a PR here:

Use 4.0.6-SNAPSHOT for latest fixes/enhancements eclipse-platform/eclipse.platform.releng.aggregator#1758

so get a better overview what would happen in the large (requires snapshot Tycho).

I would keep that in Tycho, as the patch here is incomplete and needs to be fixed.

I leave the decisions to you (or JDT team) if this change is useful so it can benefit everyone not only latest Tycho users...

Currently the generation of lamdas can change depending on when the source file is processed. This sorts the array of files (if given) to always have a predictable order independent of order given on the commandline / arguments. Relates to eclipse-jdt#1921

iloveeclipse · 2024-01-31T08:36:45Z

if this change is useful so it can benefit everyone not only latest Tycho users.

What I've meant is: as long as patch here is incomplete and breaks ecj functionality, Tycho workaround has to be used.
If the patch is OK, we will for sure like to have it in the compiler, as stable compilation behavior is important for everyone.

laeubi · 2024-01-31T08:40:58Z

If I where to fix it on the compiler level it seems worth to sort the CUs by name in the method org.eclipse.jdt.internal.compiler.Compiler.internalBeginToCompile(ICompilationUnit[], int) I'll prepare a PR for this to see if it gives similar results but that's beyond my understanding if compiler internals to judge what is a better approach.

laeubi · 2024-01-31T08:50:09Z

This is done on the Compiler /CU level

Sort by (CU) filename (if given) by for consistent output #1933

laeubi · 2024-01-31T15:56:03Z

Looking at the build results this has not produced any baseline problem eve though it would be expected it seems the change has had not the same effect as desired...

iloveeclipse · 2024-01-31T16:01:10Z

Looking at the build results this has not produced any baseline problem eve though it would be expected it seems the change has had not the same effect as desired...

This change is needed to see new compiler used for SDK: eclipse-platform/eclipse.platform.releng.aggregator#1760

laeubi mentioned this pull request Jan 30, 2024

Non deterministic compile results with lamda numbering #1921

Open

iloveeclipse requested changes Jan 30, 2024

View reviewed changes

laeubi force-pushed the issu_1921_order_input_files branch from d69ce19 to d411905 Compare January 31, 2024 07:02

laeubi force-pushed the issu_1921_order_input_files branch from d411905 to cf4727f Compare January 31, 2024 08:33

iloveeclipse approved these changes Jan 31, 2024

View reviewed changes

iloveeclipse merged commit 4dca3bb into eclipse-jdt:master Jan 31, 2024
9 checks passed

iloveeclipse mentioned this pull request Jan 31, 2024

Sort by (CU) filename (if given) by for consistent output #1933

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sort filename (if given) by name for consistent output #1922

Sort filename (if given) by name for consistent output #1922

laeubi commented Jan 30, 2024

iloveeclipse commented Jan 30, 2024

laeubi commented Jan 30, 2024

iloveeclipse left a comment

iloveeclipse commented Jan 30, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

iloveeclipse commented Jan 31, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

iloveeclipse commented Jan 31, 2024

Sort filename (if given) by name for consistent output #1922

Sort filename (if given) by name for consistent output #1922

Conversation

laeubi commented Jan 30, 2024

What it does

How to test

Author checklist

iloveeclipse commented Jan 30, 2024

laeubi commented Jan 30, 2024

iloveeclipse left a comment

Choose a reason for hiding this comment

iloveeclipse commented Jan 30, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

iloveeclipse commented Jan 31, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

laeubi commented Jan 31, 2024

iloveeclipse commented Jan 31, 2024