Revert "Jenkinsfile: Disable Quality gate for maven warnings #969"

[HannesWell]

This reverts commit 5a4d891/PR #1007 now that #969 was fixed temporarily with eclipse-platform/eclipse.platform.releng.aggregator#1672 and is permanently fixed with eclipse-equinox/equinox#403.

[github-actions]

Test Results

   594 files  ±0     594 suites  ±0   39m 56s ⏱️ + 2m 42s
 3 864 tests ±0   3 842 ✅ ±0   21 💤 ±0  1 ❌ ±0 
12 198 runs  ±0  12 037 ✅ ±0  158 💤 ±0  3 ❌ ±0 

For more details on these failures, see this check.

Results for commit a3e1dfc. ± Comparison against base commit ef98ac4.

♻️ This comment has been updated with latest results.

[laeubi]

@HannesWell as I mentioned in the issue already, these maven warnings are not due to the http service "issue" see for example one of them as an example here:

Your build strictly depends on unit org.eclipse.e4.core.services 2.4.300.v20231214-1012 that is shadowed by a reactor project, this can lead to unexpected build results!

[HeikoKlare]

I am not sure whether the fix(es) also affect #1017, but as long as that's not solved, I would even consider completely disabling the quality gate (even for errors). I currently find no value in that quality gate despite feeling bad when merging something with failing checks just because of some random occurrence and because it's not worth to invest computation time into just getting a green tick.

[laeubi]

I would even consider completely disabling the quality gate (even for errors)

Error would fail the build so there is not much a quality gate can do.

I currently find no value in that quality gate despite feeling bad when merging something with failing checks just because of some random occurrence

As explained earlier these warnings are not random. They are because the project does not completely declares the dependencies and the warning explains what is the culprit.

[iloveeclipse]

So far I haven't saw any value in quality gate as it is implemented, it is almost always failing, the root cause is always hard or impossible to understand and everyone learns to ignore any errors and merge despite them.

So please don't add more pain.

[iloveeclipse]

Note: in my company we have our "quality check". It consists of all possible static/dynamic code checks, tests and even error log evaluations.

However, the key differentiation between the "quality gate" implemented here and our "quality check" is that we add new check if and only if the current state of the branch is "green" for this check, plus we make sure to keep the state "green" after every change, or a bug is raised / PR is reverted.

Here on github exact the opposite happens, and that leads to everyone's confusion & desperation.

[laeubi]

So far I haven't saw any value in quality gate as it is implemented, it is almost always failing,

I have already explained why it is failing but it seems easier to complain that it is failing than thinking about that it shows a problem in current build setup what should be fixed. For example in PDE we do not see it always failing because there the problem is simply not present and we recently fixed the remaining recently (still some are left).

Beside that, all checks that in the past where complained about has already lead to more contributions in fixing them e.g. flapping test (kudos to @HeikoKlare that seems to be very dedicated to this), faulty javadoc or compiler warnings (I have seen a lot from @jukzi there) and also @akurtakov is often concerned and fixing a lot cross-cutting concerns.

everyone learns to ignore any errors and merge despite them.
Here on github exact the opposite happens, and that leads to everyone's confusion & desperation.

I'm not confused and I don't ignore errors, so please don't always assume your own experience/habit as general applicable and claim its for "everyone". Its even not true for the majority of contributors, as explained above a lot of people do care but working hard on improving the quality of the code.

[HeikoKlare]

Error would fail the build so there is not much a quality gate can do.

I currently find no value in that quality gate despite feeling bad when merging something with failing checks just because of some random occurrence

As explained earlier these warnings are not random. They are because the project does not completely declares the dependencies and the warning explains what is the culprit.

@laeubi We are talking about different issues: you refer to the dependency issue, while I refer to the one I have documented in #1017. The latter one is different and it both leads to random failures as well as to reported errors even though the build runs successful.

So far I haven't saw any value in quality gate as it is implemented, it is almost always failing, the root cause is always hard or impossible to understand and everyone learns to ignore any errors and merge despite them.

I fully agree with respect to the impact of the current state of the quality gate. I also fear that the any error in the Jenkins builds is argued by the current issues, even though they are not related. But even if you are willing to always properly investigate if there is a different issue with the build: it requires effort and such manual checks are always prone to accidental (unintentional) failures.

[iloveeclipse]

everyone learns to ignore any errors and merge despite them.
Here on github exact the opposite happens, and that leads to everyone's confusion & desperation.

I'm not confused and I don't ignore errors, so please don't always assume your own experience/habit as general applicable and claim its for "everyone". Its even not true for the majority of contributors

I tried to avoid pointing to anyone personally, so let's state it differently:

Everyone (except one contributor) learns to ignore any errors and everyone merges despite them.
... and that leads to everyone's (except one contributor) confusion & desperation.

as explained above a lot of people do care but working hard on improving the quality of the code.

Sure, but that work can happen also without quality gate that confuses everyone (except one contributor), on a private fork.

[laeubi]

We are talking about different issues: you refer to the dependency issue, while I refer to the one I have documented in #1017. The latter one is different and it both leads to random failures as well as to reported errors even though the build runs successful.

Javadoc is part of the build, so it can't run successful when there are errors, you probably mean that is compiles successful, but that's different. So if there is a recent example where this is reproducible it should be analyzed and fixed, I can only assume it is due the now optional nature of the javax.inject, but given that @HannesWell has removed all references from the platform it seems it should never appear anywhere so is just an indication that the javadoc seem to reference the old javax.inject instead of jakarta.inject somewhere and should be fixed in this regards.

So how does it help to make the issue invisible?

Sure, but that work can happen also without quality gate that confuses everyone (except one contributor), on a private fork.

Its very unlikely that anyone will ever do that when it is not visible and accepted by the projects verification builds as this simply states it is ok to have such issues.

[HeikoKlare]

Javadoc is part of the build, so it can't run successful when there are errors, you probably mean that is compiles successful, but that's different.

Of course that's a difference. But obviously the build can run successful (as it does most of the time, even though Javadoc generation produces errors). So we should tell the Maven build to treat the Javadoc errors as errors and not flag the build as succesful despite those errors 🙂

So if there is a recent example where this is reproducible it should be analyzed and fixed, I can only assume it is due the now optional nature of the javax.inject, but given that @HannesWell has removed all references from the platform it seems it should never appear anywhere so is just an indication that the javadoc seem to reference the old javax.inject instead of jakarta.inject somewhere and should be fixed in this regards.

I do not want to repeat everything documented in #1017. If any information is missing there, I think it makes more sense to have the discussion there. The platform code still contains javax.inject references (from my understanding, for compatibility reasons) and that's what the Javadoc generation complains about. That has been the case for weeks and did not change within the last days. You can see that in every (even the most recent) platform build log on Jenkins. The currently last successful master build was: https://ci.eclipse.org/platform/job/eclipse.platform/job/master/594/
That is one (of two) issues as reported in #1017.

[HeikoKlare]

The reason for the build not failing even though Javadoc generation fails will probably be the according parent pom configuration:

                                <failOnError>false</failOnError>

[laeubi]

You can see that in every (even the most recent) platform build log on Jenkins.
The currently last successful master build was: https://ci.eclipse.org/platform/job/eclipse.platform/job/master/594/

• https://ci.eclipse.org/platform/job/eclipse.platform/job/master/594/ did not show any javadoc errors
• https://ci.eclipse.org/platform/job/eclipse.platform/job/master/595/ failed due to failing test not because of javadoc errors
• https://ci.eclipse.org/platform/job/eclipse.platform/job/master/596/ was aborted because it was superseded not because of javadoc errors
• https://ci.eclipse.org/platform/job/eclipse.platform/job/master/597/ failed because o product can't be assembled so not because of javadoc errors

[HeikoKlare]

Not sure what you are looking at, but the logs of all builds you refer to contain errors like:

15:53:10.798 [ERROR] MavenReportException: Error while generating Javadoc: 
Exit code: 1
/home/jenkins/agent/workspace/eclipse.platform_master/runtime/bundles/org.eclipse.e4.core.di.annotations/src/org/eclipse/e4/core/di/annotations/Creatable.java:26: error: package javax.inject does not exist
@javax.inject.Qualifier
             ^
/home/jenkins/agent/workspace/eclipse.platform_master/runtime/bundles/org.eclipse.e4.core.di.annotations/src/org/eclipse/e4/core/di/annotations/Optional.java:54: error: package javax.inject does not exist
@javax.inject.Qualifier
             ^
2 errors
Command line was: /opt/tools/java/openjdk/jdk-17/17.0.2+8/bin/javadoc @options @packages

[Bananeweizen]

I would like to suggest a different change: leave both recordIssues as is, remove only the quality gate calculation (the last part of the line). That way warnings/errors are still recorded, but don't affect build output. That's far better than not recording at all, because a human can still compare the differences between certain builds, if wanted. So if someone notes an unwanted log message, she can go back through history to see when it started etc.

[HeikoKlare]

I would like to suggest a different change: leave both recordIssues as is, remove only the quality gate calculation (the last part of the line). That way warnings/errors are still recorded, but don't affect build output. That's far better than not recording at all, because a human can still compare the differences between certain builds, if wanted. So if someone notes an unwanted log message, she can go back through history to see when it started etc.

+1 for the proposal. I actually had the same in mind (still provide the results but not let them mark the whole build as unstable). Will setting unstable=false for the Jenkins maven plug-in achieve that?

[laeubi]

+1 for the proposal. I actually had the same in mind (still provide the results but not let them mark the whole build as unstable). Will setting unstable=false for the Jenkins maven plug-in achieve that?

Ever committer on the project can simply edit the file, open a PR and see what happens or consult the documentation if in doubt, Jenkins also includes a UI to get a snippet to get a snippet.

[laeubi]

Not sure what you are looking at, but the logs of all builds you refer to contain errors like:

But none of the are "randomly failing all builds" as it is claimed here, the only failing builds are not because of this error that will be fixed here:

• Add old javax.inject as an additional javadoc dependency eclipse.platform.releng.aggregator#1680

[HeikoKlare]

Not sure what you are looking at, but the logs of all builds you refer to contain errors like:

But none of the are "randomly failing all builds" as it is claimed here, the only failing builds are not because of this error that will be fixed here:

• Add old javax.inject as an additional javadoc dependency eclipse.platform.releng.aggregator#1680

Yes, recent builds have not been failing because of the Javadoc issue. I have just referred to them as every build log contains Javadoc generation errors, but not that each of them makes the build fail. Again, I have explained in #1017 what makes the build randomly fail (Javadoc errors in combination with erroneous parsing of Maven warnings). And again, if my explanation in that issue is insufficient, let me know and let us discuss it in that issue.

[jukzi]

please don't enable maven quality until maven tests are deterministic:
this build:

[HannesWell]

I understand everyone's desire to have a green and stable build but in general I consider these quality gates valuable and based on the PRs created to fix it I would say that it already has provided some values since it has unveiled flaws that were hidden as warnings. Therefore I would be in favor of fixing the warnings and enabling it again.

Regarding the exact way how to re-enable it I cannot say much since I have not yet looked into the Jenkins plugin yet.
So if anyone else wants to take over it is welcome, I just assumed it is a quick change since the org.osgi.service.http issue was resolved.

[HannesWell]

The build is now green.
Since it was suggested to not just revert to the previous state, which configuration do you suggest to use exactly?

[jukzi]

I don't get why you are so eager to get maven quality gate enabled. To me it seems like very few commits to this repo would ever produce a justified warning related to the commit, as most commits do not touch dependencies. The current maven "warnings" look still rubbish to me. They do not meet my quality gate to be even shown at all on jenkins:

[laeubi]

The current maven "warnings" look still rubbish to me. They do not meet my quality gate to be even shown at all on jenkins:

Can you explain what you want to tell us with the screenshot? Maybe you not want to click on the PARSER logfile messages

but instead on the SUMMARY link?

So instead of qualify something as rubbish maybe at least just try to understand the purpose instead of trying to find something to complain?

[laeubi]

I don't get why you are so eager to get maven quality gate enabled. To me it seems like very few commits to this repo would ever produce a justified warning related to the commit

That's mostly because you not have been looking at the messages but more about something you "don't like" for example in this particular build you will find the following:

[API WARNING] File ContextFunction.java at line 25: ContextFunction implements non-API interface IContextFunction (location: /home/jenkins/agent/workspace/eclipse.platform_PR-1046/runtime/bundles/org.eclipse.e4.core.contexts/src/org/eclipse/e4/core/contexts/ContextFunction.java)

So unless someone wants to offer a new API Tools parser to Jenkins this is for example important to see API tools errors/warnings.

[akurtakov]

Conflicts with master, build seems to have stabilized and this PR has reverts and references to a number of other things thus just closing this one. If there is further enhancement needed/wanted it would better be done in a dedicated and clear PR.