| Message ID | cover.1717742752.git.ps@pks.im (mailing list archive) |
|---|---|
| Headers | show |
| Series | ci: detect more warnings via `-Og` | expand |
Patrick Steinhardt <ps@pks.im> writes: > this is the third version of this patch series that adapts one of our CI > jobs to compile with `-Og` for improved coverage of warnings. > > Changes compared to v2: > > - Typo fixes for commit messages. > > - Replaced the `O` variable with `CFLAGS_APPEND`. If that isn't > palatable to folks then I'll drop this altogether and will inline it > into the CI script, duplicating the default CFLAGS there. > > - Start compiling with V=1 so that the change can actually be seen. It > also shouldn't clutter the job output too much given that the build > is in a collapsible section on both GitHub and GitLab. I've taken the "Python 2 - missing 'test'" on a separate topic and will merge it down to 'next' and 'master' to fast track. The "override CFLAGS += CFLAGS_APPEND" thing looks good. I am not sure how annoying people will find the V=1 output. It is irrelevant that it is in a collapsible section. What matters is if it helps those who *need* to expand that collapsible section to take a look, or if it clutteres what they have to wade through. When studying a build failure, I rarely found the exact command line given by V=1 helpful, but YMMV---while I am not 100% convinced, let's take the series as-is, because not losing information may sometimes help even when we need to visually filter out extra clutter. Thanks.
On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote: > I am not sure how annoying people will find the V=1 output. It is > irrelevant that it is in a collapsible section. What matters is if > it helps those who *need* to expand that collapsible section to take > a look, or if it clutteres what they have to wade through. > > When studying a build failure, I rarely found the exact command line > given by V=1 helpful, but YMMV---while I am not 100% convinced, let's > take the series as-is, because not losing information may sometimes > help even when we need to visually filter out extra clutter. I had the same thought. I have used V=1 for debugging, but usually debugging Makefile changes locally (i.e., why is my option not being passed correctly). I don't think I've ever wanted it for a CI run. And I do think people see the output. It may be in a collapsible section on the site, but: - you'd uncollapse that section if there is a build failure, and now your error messages are that much harder to find - if you look at the output outside of the site, you'll see the uncollapsed sections. And I usually view them in a local pager using curl[1]. I guess I won't know until I see it in action, but I have a pretty strong suspicion that it will be annoying. -Peff [1] For the curious, I use the script below to grab the logs from the last failed workflow run. It's pretty old, and I won't be surprised if there's a less ugly way to do it with the "gh" CLI tool or similar these days. -- >8 -- repo=peff/git GITHUB_TOKEN=...your.pat.here... actions() { curl -sL \ -H 'Accept: application/vnd.github.v3+json' \ -H "Authorization: token $GITHUB_TOKEN" \ "https://api.github.com/repos/$repo/actions/$1" } run=$( actions runs?status=failure | jq -r '.workflow_runs | .[] | .id' | head -1 ) actions runs/$run/jobs?per_page=100 | jq -r '.jobs | .[] | select(.conclusion == "failure") | "\(.id) \(.name)"' | while read id name; do { echo "==> $name" actions jobs/$id/logs } | less '+/not.ok|##\[error\]' done
Jeff King <peff@peff.net> writes: > On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote: > >> I am not sure how annoying people will find the V=1 output. It is >> irrelevant that it is in a collapsible section. What matters is if >> it helps those who *need* to expand that collapsible section to take >> a look, or if it clutteres what they have to wade through. >> >> When studying a build failure, I rarely found the exact command line >> given by V=1 helpful, but YMMV---while I am not 100% convinced, let's >> take the series as-is, because not losing information may sometimes >> help even when we need to visually filter out extra clutter. > > I had the same thought. I have used V=1 for debugging, but usually > debugging Makefile changes locally (i.e., why is my option not being > passed correctly). I don't think I've ever wanted it for a CI run. > > And I do think people see the output. It may be in a collapsible section > on the site, but: > > - you'd uncollapse that section if there is a build failure, and now > your error messages are that much harder to find > > - if you look at the output outside of the site, you'll see the > uncollapsed sections. And I usually view them in a local pager using > curl[1]. > > I guess I won't know until I see it in action, but I have a pretty > strong suspicion that it will be annoying. https://github.com/git/git/actions/runs/9424299208/job/25964282150#step:6:573 I _knew_ that this run will fail compiling the updated timestamp parsing logic in date.c but it still took me a while to find the exact error. I typed "date.o" in the search box, which showed 5 hits (first two are false hits to fuzz-date.o and test-date.o), with 3rd hit on l.566 "gcc -o date.o ... long long command line" 4th hit on l.594 "Makefile:2758: recipe for target 'date.o' failed" 5th hit on l.595 "make: *** [date.o] Error 1" Nitice that the error message with "date.c" is on 571 but with each line being very bloated to around 10 physical lines on screen, it is very far from either 3rd or 4th hit. So, this time it was annoying. But I suspect I'd be praising the wisdom of using V=1 if I were hunting for some breakage caused by tweaks in command line generation that broke the build or something, so I dunno.
On Sat, Jun 08, 2024 at 04:12:15PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Fri, Jun 07, 2024 at 01:47:25PM -0700, Junio C Hamano wrote: > > > >> I am not sure how annoying people will find the V=1 output. It is > >> irrelevant that it is in a collapsible section. What matters is if > >> it helps those who *need* to expand that collapsible section to take > >> a look, or if it clutteres what they have to wade through. > >> > >> When studying a build failure, I rarely found the exact command line > >> given by V=1 helpful, but YMMV---while I am not 100% convinced, let's > >> take the series as-is, because not losing information may sometimes > >> help even when we need to visually filter out extra clutter. > > > > I had the same thought. I have used V=1 for debugging, but usually > > debugging Makefile changes locally (i.e., why is my option not being > > passed correctly). I don't think I've ever wanted it for a CI run. > > > > And I do think people see the output. It may be in a collapsible section > > on the site, but: > > > > - you'd uncollapse that section if there is a build failure, and now > > your error messages are that much harder to find > > > > - if you look at the output outside of the site, you'll see the > > uncollapsed sections. And I usually view them in a local pager using > > curl[1]. > > > > I guess I won't know until I see it in action, but I have a pretty > > strong suspicion that it will be annoying. > > https://github.com/git/git/actions/runs/9424299208/job/25964282150#step:6:573 > > I _knew_ that this run will fail compiling the updated timestamp > parsing logic in date.c but it still took me a while to find the > exact error. > > I typed "date.o" in the search box, which showed 5 hits (first two > are false hits to fuzz-date.o and test-date.o), with > > 3rd hit on l.566 "gcc -o date.o ... long long command line" > 4th hit on l.594 "Makefile:2758: recipe for target 'date.o' failed" > 5th hit on l.595 "make: *** [date.o] Error 1" > > Nitice that the error message with "date.c" is on 571 but with each > line being very bloated to around 10 physical lines on screen, it is > very far from either 3rd or 4th hit. > > So, this time it was annoying. But I suspect I'd be praising the > wisdom of using V=1 if I were hunting for some breakage caused by > tweaks in command line generation that broke the build or something, > so I dunno. I'll just drop this patch for now. Patrick
Hi, this is the third version of this patch series that adapts one of our CI jobs to compile with `-Og` for improved coverage of warnings. Changes compared to v2: - Typo fixes for commit messages. - Replaced the `O` variable with `CFLAGS_APPEND`. If that isn't palatable to folks then I'll drop this altogether and will inline it into the CI script, duplicating the default CFLAGS there. - Start compiling with V=1 so that the change can actually be seen. It also shouldn't clutter the job output too much given that the build is in a collapsible section on both GitHub and GitLab. Patrick Patrick Steinhardt (4): ci: fix check for Ubuntu 20.04 Makefile: add ability to append to CFLAGS and LDFLAGS ci: compile code with V=1 ci: compile "linux-gcc-default" job with -Og Makefile | 2 ++ ci/lib.sh | 2 +- ci/run-build-and-tests.sh | 11 ++++++++++- 3 files changed, 13 insertions(+), 2 deletions(-) Range-diff against v2: 1: f91004a438 = 1: 70fa755b4f ci: fix check for Ubuntu 20.04 -: ---------- > 2: d68539834f Makefile: add ability to append to CFLAGS and LDFLAGS -: ---------- > 3: a3dfb7092a ci: compile code with V=1 2: bdf0e40a77 ! 4: c7b5b62d9c ci: compile "linux-gcc-default" job with -Og @@ Commit message We have recently noticed that our CI does not always notice variables that may be used uninitialized. While it is expected that compiler - warnings aren't perfect, this one was a but puzzling because it was + warnings aren't perfect, this one was a bit puzzling because it was rather obvious that the variable can be uninitialized. Many compiler warnings unfortunately depend on the optimization level @@ Commit message Instead, adapt the "linux-gcc-default" job to compile with `-Og`. This job is chosen because it uses the "ubuntu:latest" image and should thus have a comparatively recent compiler toolchain, and because we have - other jobs that use "ubuntu:latest" so that we do not loose coverage for + other jobs that use "ubuntu:latest" so that we do not lose coverage for warnings diagnosed only on `-O2` level. To make it easier to set up the optimization level in our CI, add @@ Commit message Signed-off-by: Patrick Steinhardt <ps@pks.im> - ## Makefile ## -@@ Makefile: endif - # tweaked by config.* below as well as the command-line, both of - # which'll override these defaults. - # Older versions of GCC may require adding "-std=gnu99" at the end. --CFLAGS = -g -O2 -Wall -+O ?= 2 -+CFLAGS = -g -O$(O) -Wall - LDFLAGS = - CC_LD_DYNPATH = -Wl,-rpath, - BASIC_CFLAGS = -I. - ## ci/run-build-and-tests.sh ## @@ ci/run-build-and-tests.sh: esac run_tests=t @@ ci/run-build-and-tests.sh: esac + # `-O2` will mask others. We thus use `-Og` here just so that we have + # at least one job with a different optimization level so that we can + # overall surface more warnings. -+ export O=g ++ export CFLAGS_APPEND=-Og + ;; linux-gcc) export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main