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

Enable Github Verification and cleanup build scripts #237

Closed
wants to merge 1 commit into from

Conversation

laeubi
Copy link
Contributor

@laeubi laeubi commented Apr 18, 2023

This do the follwoing:

  • enable the unified github verification build
  • removes obsolete workflows
  • move mandatory maven options into the maven.config file

FYI @iloveeclipse I intentionally left out the -Ptest-on-javase-19 on the Github build for now because I assume this is not mandatory?

@github-actions
Copy link
Contributor

github-actions bot commented Apr 18, 2023

Test Results

    6 files      6 suites   1h 9m 26s ⏱️
1 389 tests 1 385 ✅ 0 💤 4 ❌
4 149 runs  4 145 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit 6c993ae.

♻️ This comment has been updated with latest results.

Copy link
Member

@iloveeclipse iloveeclipse left a comment

Choose a reason for hiding this comment

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

OK, it happened what I've expected.
As long as github actions fail and reporting is as bad as it is, I would not merge this.

I personally have hard time to understand the "fail" output, and which build produced which "error" (which is not an error but test fail, but that's known issue).

I don't want / don't have time to explain every contributor on every PR that the various fails that we report are to be ignored (!!!), because later it causes that even experienced committer assume the PR is "safe" just because it fails everywhere else.

See for example eclipse-platform/eclipse.platform.ui#703 (comment)

=>

The way to proceed is to analyze fails and why they differ between maven / jenkins / official builds and to provide patches that would fix test fails or at least mute the known issues on github only.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

The way to proceed is to analyze fails and why they differ between maven / jenkins / official builds

a quick look seem to indicate the usual problem with no default JRE found on mac os I'll try to add a p2.inf for that ... as mentioned elsewhere failures are not to be ignored I have never claimed that nor is it recommended or fine in any way as it show for example that currently people on macos won't be able to execute the maven build, just as we ignored such fact in the past and said "don't worry if the (linux) build succeed" does not mean that's good in terms of code quality and build stability.

@iloveeclipse
Copy link
Member

as mentioned elsewhere failures are not to be ignored

The problem is, as long as there are multiple fails, one has to ignore them otherwise no PR in platform would be allowed to be merged. So people learn to ignore "the noise", and that is even worse as not to have build results on other platforms.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

I don't agree on the implication that people has to ignore these, even if they do. Everyone can first help stabilize the test/builds before going on with new features (and some even do so) but as long as these failures are not obvious it is very unlikely that one can effectively doing this.

The main problem is that people classify anything that gets in their way as "noise" instead of useful pointers that something is wrong and should be fixed beforehand or at least documented (!). Still there is a risk that this hides bugs in new implementations as it might adds up to the failure that currently is already revealed.

@laeubi laeubi force-pushed the unified_workflow branch from 63ceb7d to 3cd1174 Compare April 18, 2023 08:12
@iloveeclipse
Copy link
Member

I don't agree on the implication that people has to ignore these

You can agree or disagree, but the reality is that the github actions are failing since they introduction and everyone ignores the fails and merges happily to master.

I don't want this state in JDT =>

The way to proceed is to analyze fails and why they differ between maven / jenkins / official builds and to provide patches that would fix test fails or at least mute the known issues on github only.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

You can agree or disagree, but the reality is that the github actions are failing since they introduction and everyone ignores the fails and merges happily to master.

That is simply not true, platform has 1(!) failing test on mac in now because people are helping stabilize the failing tests already, so all linux and all windows test are green now ... so just because some people ignores them do not mean everyone ignores these. So its the choice of the project or the developer to ignore them, its not a natural law or "reality" ...

@iloveeclipse
Copy link
Member

See eclipse-platform/eclipse.platform.ui#721 (comment).

I don't want this state in JDT =>

The way to proceed is to analyze fails and why they differ between maven / jenkins / official builds and to provide patches that would fix test fails or at least mute the known issues on github only.

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

What should I see? That people just thing they could ignore checks? As said your project lead so your task is to lead the team to follow project rule, if the rule is "ignore any errors because we don't care" that's what happening ... and would have happened without github builds as well, as people have ignored freeze period (first by accident, then by intend).

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

alright this already fixed a lot of the failing one, lets see whats next...

@laeubi
Copy link
Contributor Author

laeubi commented Apr 18, 2023

Seems now a JRE is found but not the right one:

test requires a JVM >= 9

junit.framework.AssertionFailedError: test requires a JVM >= 9
	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.debug.tests.core.ArgumentTests.testOutput(ArgumentTests.java:369)
	at org.eclipse.jdt.debug.tests.core.ArgumentTests.testWithVMArg(ArgumentTests.java:321)
	at org.eclipse.jdt.debug.tests.core.ArgumentTests.testVMArgSimpleQuotes(ArgumentTests.java:164)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)
	at junit.framework.TestCase.runTest(TestCase.java:177)
	at junit.framework.TestCase.runBare(TestCase.java:142)
	at org.eclipse.jdt.debug.tests.AbstractDebugTest.runBare(AbstractDebugTest.java:2678)
	at junit.framework.TestResult$1.protect(TestResult.java:122)
	at junit.framework.TestResult.runProtected(TestResult.java:142)
	at junit.framework.TestResult.run(TestResult.java:125)
	at junit.framework.TestCase.run(TestCase.java:130)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at junit.framework.TestSuite.run(TestSuite.java:236)
	at junit.framework.TestSuite.runTest(TestSuite.java:241)
	at org.eclipse.jdt.debug.tests.DebugSuite$1.run(DebugSuite.java:61)
	at java.base/java.lang.Thread.run(Thread.java:833)

@iloveeclipse do you have an idea what can make macOS fragment think this is desired? Actually we make Java 8, 11, 17 (default) available but it chooses the Java 8 as default.

@iloveeclipse
Copy link
Member

Actually we make Java 8, 11, 17 (default) available but it chooses the Java 8 as default.

Just leave 17 please.

do you have an idea what can make macOS fragment think this is desired?

I have no idea how and who resolves what on macOS.

@iloveeclipse
Copy link
Member

I intentionally left out the -Ptest-on-javase-19 on the Github build for now because I assume this is not mandatory?

I believe it was needed because of new API's provided by Java 19

@akurtakov
Copy link
Contributor

Would you please rebase so we can see current status here?

@laeubi
Copy link
Contributor Author

laeubi commented Dec 2, 2023

Would you please rebase so we can see current status here?

Done.

This do the follwoing:
- enable the unified github verification build
- removes obsolete workflows
- move mandatory maven options into the maven.config file
- add p2.inf to pull in fragment for macos launches
@akurtakov
Copy link
Contributor

@laeubi Do you plan to finish this one soon or it should be closed?

@laeubi
Copy link
Contributor Author

laeubi commented Oct 4, 2024

@laeubi Do you plan to finish this one soon or it should be closed?

Actually it is more the choice of JDT team... I can only offer the setup but I can'T debug possible failures or decide if its usefull for JDT or not.

@akurtakov
Copy link
Contributor

As contributor can not work on getting clean build, PL said without it it's a no-go and no one else stepped up to do anything in year and a half it's obvious this is not going anywhere. Closing.

@akurtakov akurtakov closed this Oct 4, 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.

3 participants