# [PATCH 1/2] ci(github): use grouping also in the `win-build` job 11 messages from 2022-06-09 to 2022-06-13. Participants: Johannes Schindelin via GitGitGadget, Ævar Arnfjörð Bjarmason, Junio C Hamano. Thread: https://gitlist.dev/t/57976 ## Johannes Schindelin via GitGitGadget, 2022-06-09 11:32 Subject: [PATCH 1/2] ci(github): use grouping also in the `win-build` job Message-ID: <5212c5ec474b9441046203da363f2205795f213e.1654774347.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/5212c5ec474b9441046203da363f2205795f213e.1654774347.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin We already do the same when building Git in all the other jobs. This will allow us to piggy-back on top of grouping to mark up compiler errors in the next commit. Signed-off-by: Johannes Schindelin --- ci/make-test-artifacts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/make-test-artifacts.sh b/ci/make-test-artifacts.sh index 646967481f6..74141af0cc7 100755 --- a/ci/make-test-artifacts.sh +++ b/ci/make-test-artifacts.sh @@ -7,6 +7,6 @@ mkdir -p "$1" # in case ci/lib.sh decides to quit early . ${0%/*}/lib.sh -make artifacts-tar ARTIFACTS_DIRECTORY="$1" +group Build make artifacts-tar ARTIFACTS_DIRECTORY="$1" check_unignored_build_artifacts -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-09 11:32 Subject: [PATCH 0/2] ci(GitHub): mark up compile errors, too Message-ID: URL: https://gitlist.dev/e/pull.1253.git.1654774347.gitgitgadget%40gmail.com ``` Just like we mark up test failures, it makes sense to mark up compile errors, too. In a sense, it makes even more sense with compile errors than with test failures because we can link directly to the corresponding source code in the former case (if said code has been touched by the Pull Request, that is). The only downside is that this link currently is kind of misleading if the Pull Request did not even touch the offending source code (such as was the case when a GCC upgrade in Git for Windows' SDK all of a sudden pointed out problems in the source code that had existed for a long time already). We will see how the GitHub Actions engineers will develop this feature further. This patch series is based on js/ci-github-workflow-markup. Which also serves as an example how this looks like if the offending source code was not touched by the Pull Request: https://github.com/gitgitgadget/git/actions/runs/2461737185 because it still triggers the above-referenced GCC build failure. Johannes Schindelin (2): ci(github): use grouping also in the `win-build` job ci(github): also mark up compile errors ci/lib.sh | 10 ++++++++-- ci/make-test-artifacts.sh | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) base-commit: 3069f2a6f4c38e7e599067d2e4a8e31b4f53e2d3 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1253%2Fdscho%2Fci-mark-up-compile-failures-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1253/dscho/ci-mark-up-compile-failures-v1 Pull-Request: https://github.com/gitgitgadget/git/pull/1253 -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-09 11:32 Subject: [PATCH 2/2] ci(github): also mark up compile errors Message-ID: <19d6e34f038121b927cdfacc3c4ae5abd1791415.1654774347.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/19d6e34f038121b927cdfacc3c4ae5abd1791415.1654774347.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin When GCC produces those helpful errors, we will want to present them in the GitHub workflow runs in the most helpful manner. To that end, we want to use workflow commands to render errors and warnings: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions In the previous commit, we ensured that grouping is used for the build in all jobs, and this allows us to piggy-back onto the `group` function to transmogrify the output. Note: If `set -o pipefail` was available, we could do this in a little more elegant way. But since some of the steps are run using `dash`, we have to do a little `{ ...; echo $? >exit.status; } | ...` dance. Signed-off-by: Johannes Schindelin --- ci/lib.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index 2f6d9d26e40..b747e34842c 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -29,8 +29,14 @@ else set +x begin_group "$1" shift - "$@" - res=$? + # work around `dash` not supporting `set -o pipefail` + { + "$@" 2>&1 + echo $? >exit.status + } | + sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2 line=\3::\1/' + res=$(cat exit.status) + rm exit.status end_group return $res } -- gitgitgadget ``` ## Ævar Arnfjörð Bjarmason, 2022-06-09 13:47 Subject: Re: [PATCH 2/2] ci(github): also mark up compile errors Message-ID: <220609.864k0ut010.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220609.864k0ut010.gmgdl%40evledraar.gmail.com In-Reply-To: <19d6e34f038121b927cdfacc3c4ae5abd1791415.1654774347.git.gitgitgadget@gmail.com> ``` On Thu, Jun 09 2022, Johannes Schindelin via GitGitGadget wrote: > From: Johannes Schindelin > > When GCC produces those helpful errors, we will want to present them in > the GitHub workflow runs in the most helpful manner. To that end, we > want to use workflow commands to render errors and warnings: > https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions The docs you're linking to state: ::warning file={name},line={line},endLine={endLine},title={title}::{message} But here... > + sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2 line=\3::\1/' You seem to omit the comma, and the CI output itself seems to note the filename as "compat/win32/syslog.c line=53#L1". I haven't tested, but is this the issue you noted in the CL as "the only downside"? I.e. the link to the source code is nonsensical in that CI output, it links to the diff of the PR itself. But the GH docs say "associate the message with a particular file in your repository.", so it would seem that there should be a way to link to the file at that revision, not only if it was altered in the given commit. On the "sed" one-liner, at least GCC supports emitting JSON error output: https://stackoverflow.com/questions/36657869/how-do-i-dump-gcc-warnings-into-a-structured-format You don't fill in "column" now, but if you used that presumably that would be easy, and more useful. It seems clang also supports it, but not any easily machine-readable format: https://clang.llvm.org/docs/UsersManual.html#cmdoption-fdiagnostics-format ``` ## Junio C Hamano, 2022-06-09 23:56 Subject: Re: [PATCH 0/2] ci(GitHub): mark up compile errors, too Message-ID: URL: https://gitlist.dev/e/xmqqbkv1gzmy.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > Just like we mark up test failures, it makes sense to mark up compile > errors, too. > > In a sense, it makes even more sense with compile errors than with test > failures because we can link directly to the corresponding source code in Absolutely ;-). ``` ## Junio C Hamano, 2022-06-10 16:30 Subject: Re: [PATCH 2/2] ci(github): also mark up compile errors Message-ID: URL: https://gitlist.dev/e/xmqqzgikfpm8.fsf%40gitster.g In-Reply-To: <19d6e34f038121b927cdfacc3c4ae5abd1791415.1654774347.git.gitgitgadget@gmail.com> ``` "Johannes Schindelin via GitGitGadget" writes: > + sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2 line=\3::\1/' https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#setting-a-warning-message seems to use comma as field separator, not SP, for files and lines. We'll see if they are equivalent without getting documented soon, as I will be adding this to my tree for 'seen' today. We should throw a build failure in to see its effect ;-) ``` ## Johannes Schindelin via GitGitGadget, 2022-06-13 13:13 Subject: [PATCH v2 2/2] ci(github): also mark up compile errors Message-ID: <34daf06bb710434a2392b3cb91d35047565d5602.1655125988.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/34daf06bb710434a2392b3cb91d35047565d5602.1655125988.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin When GCC produces those helpful errors, we will want to present them in the GitHub workflow runs in the most helpful manner. To that end, we want to use workflow commands to render errors and warnings: https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions In the previous commit, we ensured that grouping is used for the build in all jobs, and this allows us to piggy-back onto the `group` function to transmogrify the output. Note: If `set -o pipefail` was available, we could do this in a little more elegant way. But since some of the steps are run using `dash`, we have to do a little `{ ...; echo $? >exit.status; } | ...` dance. Signed-off-by: Johannes Schindelin --- ci/lib.sh | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/ci/lib.sh b/ci/lib.sh index 2f6d9d26e40..aa7e979a0bf 100755 --- a/ci/lib.sh +++ b/ci/lib.sh @@ -29,8 +29,14 @@ else set +x begin_group "$1" shift - "$@" - res=$? + # work around `dash` not supporting `set -o pipefail` + ( + "$@" 2>&1 + echo $? >exit.status + ) | + sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/' + res=$(cat exit.status) + rm exit.status end_group return $res } -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-13 13:13 Subject: [PATCH v2 0/2] ci(GitHub): mark up compile errors, too Message-ID: URL: https://gitlist.dev/e/pull.1253.v2.git.1655125988.gitgitgadget%40gmail.com In-Reply-To: ``` Just like we mark up test failures, it makes sense to mark up compile errors, too. In a sense, it makes even more sense with compile errors than with test failures because we can link directly to the corresponding source code in the former case (if said code has been touched by the Pull Request, that is). The only downside is that this link currently is kind of misleading if the Pull Request did not even touch the offending source code (such as was the case when a GCC upgrade in Git for Windows' SDK all of a sudden pointed out problems in the source code that had existed for a long time already). We will see how the GitHub Actions engineers will develop this feature further. This patch series is based on js/ci-github-workflow-markup. Which also serves as an example how this looks like if the offending source code was not touched by the Pull Request: https://github.com/dscho/git/actions/runs/2477526645 because it still triggers the above-referenced GCC build failure. Changes since v1: * Using a comma in the workflow command now, as described in the official documentation ;-) (Thank you, Ævar) * The curly bracket construct was replaced by a proper subshell, to avoid jumbled output and a race where the exit.status file could be read before it was written. Johannes Schindelin (2): ci(github): use grouping also in the `win-build` job ci(github): also mark up compile errors ci/lib.sh | 10 ++++++++-- ci/make-test-artifacts.sh | 2 +- 2 files changed, 9 insertions(+), 3 deletions(-) base-commit: 3069f2a6f4c38e7e599067d2e4a8e31b4f53e2d3 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1253%2Fdscho%2Fci-mark-up-compile-failures-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1253/dscho/ci-mark-up-compile-failures-v2 Pull-Request: https://github.com/gitgitgadget/git/pull/1253 Range-diff vs v1: 1: 5212c5ec474 = 1: 5212c5ec474 ci(github): use grouping also in the `win-build` job 2: 19d6e34f038 ! 2: 34daf06bb71 ci(github): also mark up compile errors @@ ci/lib.sh: else - "$@" - res=$? + # work around `dash` not supporting `set -o pipefail` -+ { ++ ( + "$@" 2>&1 + echo $? >exit.status -+ } | -+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2 line=\3::\1/' ++ ) | ++ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/' + res=$(cat exit.status) + rm exit.status end_group -- gitgitgadget ``` ## Johannes Schindelin via GitGitGadget, 2022-06-13 13:13 Subject: [PATCH v2 1/2] ci(github): use grouping also in the `win-build` job Message-ID: <5212c5ec474b9441046203da363f2205795f213e.1655125988.git.gitgitgadget@gmail.com> URL: https://gitlist.dev/e/5212c5ec474b9441046203da363f2205795f213e.1655125988.git.gitgitgadget%40gmail.com In-Reply-To: ``` From: Johannes Schindelin We already do the same when building Git in all the other jobs. This will allow us to piggy-back on top of grouping to mark up compiler errors in the next commit. Signed-off-by: Johannes Schindelin --- ci/make-test-artifacts.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci/make-test-artifacts.sh b/ci/make-test-artifacts.sh index 646967481f6..74141af0cc7 100755 --- a/ci/make-test-artifacts.sh +++ b/ci/make-test-artifacts.sh @@ -7,6 +7,6 @@ mkdir -p "$1" # in case ci/lib.sh decides to quit early . ${0%/*}/lib.sh -make artifacts-tar ARTIFACTS_DIRECTORY="$1" +group Build make artifacts-tar ARTIFACTS_DIRECTORY="$1" check_unignored_build_artifacts -- gitgitgadget ``` ## Junio C Hamano, 2022-06-13 17:08 Subject: Re: [PATCH v2 0/2] ci(GitHub): mark up compile errors, too Message-ID: URL: https://gitlist.dev/e/xmqq35g88p9y.fsf%40gitster.g In-Reply-To: ``` "Johannes Schindelin via GitGitGadget" writes: > * The curly bracket construct was replaced by a proper subshell, to avoid > jumbled output and a race where the exit.status file could be read before > it was written. I do prefer () when making a subshell in a case like this (e.g. upstream of a pipe), so I am happy with this version, but the above is curious. I am not sure how "jumbled output" is possible, let alone "reading exit.status before it is written". The output goes to sed to be processed either way, nobody else other than "$@" produces such an output from there, and sed would not exit until it finishes reading from the upstream so res=$(cat exit.status) won't kick in before the upstream exits. Anyway, thanks, will queue. > Johannes Schindelin (2): > ci(github): use grouping also in the `win-build` job > ci(github): also mark up compile errors > > ci/lib.sh | 10 ++++++++-- > ci/make-test-artifacts.sh | 2 +- > 2 files changed, 9 insertions(+), 3 deletions(-) > > > base-commit: 3069f2a6f4c38e7e599067d2e4a8e31b4f53e2d3 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1253%2Fdscho%2Fci-mark-up-compile-failures-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1253/dscho/ci-mark-up-compile-failures-v2 > Pull-Request: https://github.com/gitgitgadget/git/pull/1253 > > Range-diff vs v1: > > 1: 5212c5ec474 = 1: 5212c5ec474 ci(github): use grouping also in the `win-build` job > 2: 19d6e34f038 ! 2: 34daf06bb71 ci(github): also mark up compile errors > @@ ci/lib.sh: else > - "$@" > - res=$? > + # work around `dash` not supporting `set -o pipefail` > -+ { > ++ ( > + "$@" 2>&1 > + echo $? >exit.status > -+ } | > -+ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2 line=\3::\1/' > ++ ) | > ++ sed 's/^\(\([^ ]*\):\([0-9]*\):\([0-9]*:\) \)\(error\|warning\): /::\5 file=\2,line=\3::\1/' > + res=$(cat exit.status) > + rm exit.status > end_group ``` ## Ævar Arnfjörð Bjarmason, 2022-06-13 22:41 Subject: Re: [PATCH v2 0/2] ci(GitHub): mark up compile errors, too Message-ID: <220614.86tu8oyxu9.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220614.86tu8oyxu9.gmgdl%40evledraar.gmail.com In-Reply-To: ``` On Mon, Jun 13 2022, Johannes Schindelin via GitGitGadget wrote: > Just like we mark up test failures, it makes sense to mark up compile > errors, too. > > In a sense, it makes even more sense with compile errors than with test > failures because we can link directly to the corresponding source code in > the former case (if said code has been touched by the Pull Request, that > is). The only downside is that this link currently is kind of misleading if > the Pull Request did not even touch the offending source code (such as was > the case when a GCC upgrade in Git for Windows' SDK all of a sudden pointed > out problems in the source code that had existed for a long time already). > We will see how the GitHub Actions engineers will develop this feature > further. > > This patch series is based on js/ci-github-workflow-markup. Which also > serves as an example how this looks like if the offending source code was > not touched by the Pull Request: > https://github.com/dscho/git/actions/runs/2477526645 because it still > triggers the above-referenced GCC build failure. > > Changes since v1: > > * Using a comma in the workflow command now, as described in the official > documentation ;-) (Thank you, Ævar) You're welcome! > * The curly bracket construct was replaced by a proper subshell, to avoid > jumbled output and a race where the exit.status file could be read before > it was written. > > Johannes Schindelin (2): > ci(github): use grouping also in the `win-build` job > ci(github): also mark up compile errors It's still genuinely unclear to me what exactly the expected before/after result is, and I wish the 2/2 commit would discuss it. So, in v1 we had this: https://github.com/gitgitgadget/git/actions/runs/2461737185 Where the *summary* for the CI said e.g. "syslog.c line=53#L1", so that was the "needs a comma" bug, now it says syslog.c#L53 instead: https://github.com/dscho/git/actions/runs/2477526645 (your link above). So that's good. But re my earlier comment where I asked/wondered if fixing that would link to the source file at line 53 it still seems to just link to the diff. Is that a bug? The desired result? If the commit was modifying syslog.c would the link work? Clearly an end result where we link to the source file/lines at the rev we're testing is much more useful. I found this discussion: https://github.community/t/are-github-actions-notice-warning-error-annotations-broken/225674 Which has a link to an example run at: https://github.com/IronTooch-ColdStorage/Github-AnnotationTest/actions/runs/1782265048 So isn't this for creating "annotations" for just the regions that would be involved in your diff? I.e. it shows a notice for the line(s) involved in the diff itself, but presumably nothing else? If that's the case I think it would be much more useful to just e.g. wrap $(CC) in some "tee"-like command to spew its output somewhere, and then have a "step" where we extract the warnings/errors emitted, and emit URLs you could click on, unless there's some way to make the GitHub UX emit the same information. I.e. it'll be quite hit & miss whether the annotation will show up in the diff, the compiler will often warn about a line some distance away from the change made, e.g. if a variable is made unused. Unless the intent is only to aggregate them on the summary page, but then why do we need to link to the "line" at all, which will at best work unreliably, and at worst be actively misleading. In any case, needing to do less reading of the tea leaves would be nice, i.e. if the commit message explain what the desired change is exactly, and how it should be handling these cases. Thanks. ```