Skip to content

Conversation

@dscho
Copy link
Member

@dscho dscho commented Nov 17, 2025

It has become quite hard to debug CI failures when they happen in one of the Dockerized jobs, as the actual test failures are now hidden. This was most likely an oversight when 2a21098 (github: adapt containerized jobs to be rootless, 2025-01-10) was merged in 2bf3c7f (Merge branch 'ps/ci-misc-updates', 2025-02-06), v2.49.0-rc0~55, and I had reported this as a regression in https://lore.kernel.org/git/e45b9487-b3ae-ed85-fd07-c92cfbf47cbb@gmx.de/. Seeing no movement on my report, and having the pressure of newly-failing tests during the v2.52.0-rc0 rebase of Git for Windows, I was kind of forced into fixing this in Git for Windows. Here I upstream the fix.

cc: Jeff King peff@peff.net

@dscho dscho self-assigned this Nov 17, 2025
The quality of tests/test suites does not show as much when there are no
breakages as in the amount of time required after bugs trigger test
failures before the bugs can be identified, analyzed and resolved.

As such, it is an unfortunate side effect of 2a21098 (github: adapt
containerized jobs to be rootless, 2025-01-10) that the output of failed
test cases, which was shown before that change directly in the build
logs, is now no longer shown at all.

The reason is a side effect of trying to run the build and the tests
with permissions other than the `root` user, but without providing the
prerequisite permissions to signal what tests failed and whose output
hence needs to be included in the logs.

The way this signaling works is for the workflow to write into
special-purpose files whose path is specific to the current workflow
step and which can be accessed via the `$GITHUB_ENV` environment
variable, which differs between workflow steps. It is this file that is
missing write permission for the `builder` user that was introduced in
above-mentioned commit.

The solution is simple: make the file world-writable.

Technically, this write permission should be removed after the step has
completed, if proper security practices were to be upheld, but since
nothing uses that file again, it does not matter, and the fix is more
succinct this way.

This commit is best viewed with `--color-words`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho dscho force-pushed the fix-failure-reporting-in-dockerized-ci branch from 88b7029 to 516e5a3 Compare November 17, 2025 17:02
@dscho
Copy link
Member Author

dscho commented Nov 17, 2025

/submit

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

Submitted as pull.2003.git.1763399064983.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1

To fetch this version to local tag pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The quality of tests/test suites does not show as much when there are no
> breakages as in the amount of time required after bugs trigger test
> failures before the bugs can be identified, analyzed and resolved.
>
> As such, it is an unfortunate side effect of 2a21098b98a (github: adapt
> containerized jobs to be rootless, 2025-01-10) that the output of failed
> test cases, which was shown before that change directly in the build
> logs, is now no longer shown at all.
>
> The reason is a side effect of trying to run the build and the tests
> with permissions other than the `root` user, but without providing the
> prerequisite permissions to signal what tests failed and whose output
> hence needs to be included in the logs.
>
> The way this signaling works is for the workflow to write into
> special-purpose files whose path is specific to the current workflow
> step and which can be accessed via the `$GITHUB_ENV` environment
> variable, which differs between workflow steps. It is this file that is
> missing write permission for the `builder` user that was introduced in
> above-mentioned commit.
>
> The solution is simple: make the file world-writable.

I expected to see a+w not o+w from this statement; as long as it
works I have no strong objections, but if I saw o+w without the
above explanation I would probably have wondered who are in the
group that we do not want this file touched by.

> Technically, this write permission should be removed after the step has
> completed, if proper security practices were to be upheld, but since
> nothing uses that file again, it does not matter, and the fix is more
> succinct this way.
>
> This commit is best viewed with `--color-words`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2003%2Fdscho%2Ffix-failure-reporting-in-dockerized-ci-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2003/dscho/fix-failure-reporting-in-dockerized-ci-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2003
>
>  .github/workflows/main.yml | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index 816d5a34c4..ca7cc2984f 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -433,7 +433,7 @@ jobs:
>      - run: ci/install-dependencies.sh
>      - run: useradd builder --create-home
>      - run: chown -R builder .
> -    - run: sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh
> +    - run: chmod o+w $GITHUB_ENV && sudo --preserve-env --set-home --user=builder ci/run-build-and-tests.sh
>      - name: print test failures
>        if: failure() && env.FAILED_TEST_ARTIFACTS != ''
>        run: sudo --preserve-env --set-home --user=builder ci/print-test-failures.sh

Thanks.  Will apply.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 17, 2025

This patch series was integrated into seen via git@6994512.

@gitgitgadget gitgitgadget bot added the seen label Nov 17, 2025
@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

On the Git mailing list, Jeff King wrote (reply to this):

On Mon, Nov 17, 2025 at 05:04:24PM +0000, Johannes Schindelin via GitGitGadget wrote:

> The way this signaling works is for the workflow to write into
> special-purpose files whose path is specific to the current workflow
> step and which can be accessed via the `$GITHUB_ENV` environment
> variable, which differs between workflow steps. It is this file that is
> missing write permission for the `builder` user that was introduced in
> above-mentioned commit.

Thanks for fixing this. It bit me recently, but I hadn't had time to
look at it yet.

BTW, I ran into a similar issue (no useful output from a failed test) in
the windows-meson job, but the cause is totally different there:

  https://lore.kernel.org/git/20251118093221.GA530337@coredump.intra.peff.net/

-Peff

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

User Jeff King <peff@peff.net> has been added to the cc: list.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This patch series was integrated into seen via git@0e32ceb.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This branch is now known as js/ci-show-breakage-in-dockerized-jobs.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 18, 2025

This patch series was integrated into seen via git@ae8eebd.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 19, 2025

This patch series was integrated into seen via git@980b47f.

@gitgitgadget
Copy link

gitgitgadget bot commented Nov 20, 2025

There was a status update in the "New Topics" section about the branch js/ci-show-breakage-in-dockerized-jobs on the Git mailing list:

Dockerised jobs at the GitHub Actions CI have been taught to show
more details of failed tests.

Will merge to 'next'?
cf. <xmqqpl9gike6.fsf@gitster.g>
source: <pull.2003.git.1763399064983.gitgitgadget@gmail.com>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant