Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#62530 closed defect (fixed)

pull request CI checks are marked successful even when they fail

Reported by: ryandesign (Ryan Carsten Schmidt) Owned by: admin@…
Priority: Normal Milestone:
Component: buildbot/mpbb Version:
Keywords: Cc: herbygillot (Herby Gillot), mascguy (Christopher Nielsen), l2dy (Zero King), cjones051073 (Chris Jones)
Port:

Description

For example:

Run set -eu
Cleaning up between ports
Installing dependencies for py39-graphene
Error: Failed to install dependencies for py39-graphene
/Users/runner/work/_temp/cc7409c9-0bbd-4229-a618-2ade88004038.sh: line 4:  4080 Terminated: 15          tail -f "$workdir/logs/dependencies-progress.txt" 2> /dev/null
Cleaning up between ports
Installing dependencies for py-graphene
Error: Failed to install dependencies for py-graphene
/Users/runner/work/_temp/cc7409c9-0bbd-4229-a618-2ade88004038.sh: line 4:  4148 Terminated: 15          tail -f "$workdir/logs/dependencies-progress.txt" 2> /dev/null
Cleaning up between ports
Installing dependencies for py37-graphene
Error: Failed to install dependencies for py37-graphene
/Users/runner/work/_temp/cc7409c9-0bbd-4229-a618-2ade88004038.sh: line 4:  4211 Terminated: 15          tail -f "$workdir/logs/dependencies-progress.txt" 2> /dev/null
Cleaning up between ports
Installing dependencies for py38-graphene
/Users/runner/work/_temp/cc7409c9-0bbd-4229-a618-2ade88004038.sh: line 56:  4275 Terminated: 15          tail -f "$workdir/logs/dependencies-progress.txt" 2> /dev/null
Error: Failed to install dependencies for py38-graphene
Make logfiles readable

Change History (15)

comment:1 Changed 3 years ago by neverpanic (Clemens Lang)

Cc: herbygillot added

This was a conscious decision to not fail a CI build because the dependencies failed to install, since we time and time merged changes with failed CI builds that only failed due to a problem in a dependency, not in the actual port itself.

Of course, in this particular example, the person that merged this should have noticed the lint warnings about the unknown dependencies that were posted as comments to the change by the CI system. Developers should not just ignore such warnings when merging pull requests.

The fix is adding fail=1 between the lines https://github.com/macports/macports-ports/blob/master/.github/azure-workflows/main.yml#L135-L136 and https://github.com/macports/macports-ports/blob/master/.github/workflows/main.yml#L140-L141.

comment:2 Changed 3 years ago by ryandesign (Ryan Carsten Schmidt)

Could CI consider a build failed if lint produces errors (not just warnings)?

comment:3 Changed 3 years ago by mojca (Mojca Miklavec)

I recently made a PR with tons of different python packages (just for the sake of adding py39 subport) in addition to the main package that switched from python 3.8 to 3.9. I nearly oversaw the fact that some packages failed to build, and the log was so long that it was basically impossible to figure out what built and what didn't. I only tested some of the packages, but everything was green, so I felt comfortable merging and almost did that, I only later noticed livecheck warnings.

I fail to understand why it's better to deliberately mark a build a successful. If a dependency fails, the main port won't even be checked. At all. And yet it will be shown in green. If the build fails and is marked with the red color, it's still straightforward to check why the build fail and deliberately decide that it's still ok to merge something. It's not like we prevent merging failed builds.

Combined with the fact that we are no longer receiving emails with build failures, that's a perfect recipe to never notice a failed build: neither before nor after the merge.

comment:4 Changed 3 years ago by neverpanic (Clemens Lang)

Unfortunately the return code of port lint is the same independent of whether it detected errors or warnings, so failing on error only isn't simple.

Feel free to make the change I described to fix the behavior as discussed. I just didn't have time to do it myself.

comment:5 Changed 3 years ago by ryandesign (Ryan Carsten Schmidt)

Cc: mascguy added

Has duplicate #62978.

comment:6 Changed 3 years ago by mascguy (Christopher Nielsen)

This is definitely a bit frustrating. Thankfully I always look at the build times for each CI job, before concluding they were successful. And if they take less than 5 minutes, that's usually a pretty good clue that something isn't right.

Others may not scrutinize such things as closely, however.

Last edited 3 years ago by mascguy (Christopher Nielsen) (previous) (diff)

comment:7 Changed 3 years ago by l2dy (Zero King)

Cc: l2dy added

comment:8 Changed 3 years ago by cjones051073 (Chris Jones)

Cc: cjones051073 added

comment:9 Changed 3 years ago by cjones051073 (Chris Jones)

I can understand the rationale for doing this when it is an intermittent problem, due to dependencies that do not exist, but I am seeing the above errors for all PRs at the moment, when they seem to fail for deps that do exist. See e.g.

https://github.com/macports/macports-ports/pull/11189

2021-05-31T09:24:25.8033970Z Installing dependency (1 of 440) 'bzip2' with variants '' (requesting '') ... [FAIL] (archivefetch)
2021-05-31T09:24:25.8073620Z ##[endgroup]
2021-05-31T09:24:25.8074850Z ##[error]Failed to install dependencies for gnss-sdr
2021-05-31T09:24:25.8101400Z /Users/runner/work/_temp/db2e5a8e-16ad-4272-a4bd-3e2473590f36.sh: line 4:  1383 Terminated: 15          tail -f "$workdir/logs/dependencies-progress.txt" 2> /dev/null

bzip2 definitely does exist.

It seems there is a real issue with the CI builds at the moment, and the behaviour of still marking them as OK is hiding this problem.

comment:10 in reply to:  9 Changed 3 years ago by mascguy (Christopher Nielsen)

Replying to cjones051073:

I can understand the rationale for doing this when it is an intermittent problem, due to dependencies that do not exist, but I am seeing the above errors for all PRs at the moment, when they seem to fail for deps that do exist. See e.g.

[...]

It seems there is a real issue with the CI builds at the moment, and the behaviour of still marking them as OK is hiding this problem.

Agreed, this issue has rendered the CI builds useless.

Ryan/Clemens, is it possible to fix this, even if a temporary workaround is necessary...?

comment:11 Changed 3 years ago by cjones051073 (Chris Jones)

Last edited 3 years ago by ryandesign (Ryan Carsten Schmidt) (previous) (diff)

comment:12 Changed 3 years ago by cjones051073 (Chris Jones)

note the above doesn't fix the issue of why builds are now failing to install dependencies, but at least the issue will no longer be hidden.

comment:13 Changed 3 years ago by ryandesign (Ryan Carsten Schmidt)

Resolution: fixed
Status: newclosed

So... that resolves this ticket, yes?

comment:14 Changed 3 years ago by cjones051073 (Chris Jones)

Well yes, but someone should also look into why the builds are now all failing.

comment:15 Changed 3 years ago by ryandesign (Ryan Carsten Schmidt)

This ticket is not about that, and I'm sure I explained elsewhere that it was due to [847208cf7a361d2ed6dc5f4f03184068690958d2/macports-ports] (for which I will try a different solution) and [c792d69107dafe324d341408e548eff87e7a356b/macports-ports] (which I will revert now that it is a new month).

Note: See TracTickets for help on using tickets.