search

shadow-maint / shadow

Upstream shadow tree
Other
290 stars 228 forks source link

Use trap(1) to show the testsuite log #957

Closed alejandro-colomar closed 5 months ago

alejandro-colomar commented 5 months ago

Otherwise, 'cat testsuite.log' isn't run, since 'set -e' aborts the script earlier.

alejandro-colomar commented 5 months ago

I've tested it manually, by running these same commands in a docker container.

ikerexxe commented 5 months ago

Reading the logs directly in Github WebUI is very painful. Can you upload them? You have an example at https://github.com/shadow-maint/shadow/blob/master/.github/workflows/runner.yml#L102. This will provide all the logs in a compressed archive in the checks section of the PR.

By the way, I think we are already using v4 for this action, so use that one instead of v3.

alejandro-colomar commented 5 months ago

Reading the logs directly in Github WebUI is very painful. Can you upload them? You have an example at https://github.com/shadow-maint/shadow/blob/master/.github/workflows/runner.yml#L102. This will provide all the logs in a compressed archive in the checks section of the PR.

By the way, I think we are already using v4 for this action, so use that one instead of v3.

Ahh, so there's a way to see cmocka logs? I was quite frustrated in the past when cmocka reported a test failure because I didn't know how to see the logs, and had to manually reproduce the entire build inside a docker container and then cat the log file. :D

But, I don't think that's working as intended. See for example:

(BTW, that PR is expected to fail. The reason is the broken strtoi(3bsd) from libbsd. So, if you want to test a patch for a fix, that would be a good PR to test it.)

alejandro-colomar commented 5 months ago

Reading the logs directly in Github WebUI is very painful.

BTW, this reminds me of wanting shell scripts that I can run locally to produce the same stuff that we have on github, and safely. When the string-to-numeric hardening patches are applied, this might be my next project. :)

And BTW, here's the CI/CD script I wrote for the Linux man-pages on my server. It might be useful:

$ ssh www cat /srv/src/alx/linux/man-pages/man-pages.git/hooks/post-update
#!/bin/bash

set -uo pipefail;

cd /home/alx/src/linux/man-pages/man-pages/.bare.git/;

unset $(git rev-parse --local-env-vars);
git fetch srv >/dev/null;

export LANG=C.utf8;

test "$1" = "refs/heads/main" \
&& (
    cd /home/alx/src/linux/man-pages/man-pages/main/;

    git reset srv/main --hard >/dev/null;

    nohup sh <<__EOF__ >/dev/null 2>&1 &
        make build-book _LMB=/srv/www/share/dist/man-pages/git/HEAD/man-pages-HEAD.pdf;
__EOF__
)

test "$1" = "refs/heads/contrib" \
&& (
    set -Ee;

    cd /home/alx/src/linux/man-pages/man-pages/contrib/;

    git reset srv/contrib --hard >/dev/null;

    make_opts='';
    make_opts="$make_opts -j4";
    make_opts="$make_opts -Otarget";
    make_opts="$make_opts --no-print-directory";
    make_opts="$make_opts DISTNAME=man-pages-contrib";

    make_target()
    {
        make $make_opts "$@" 2>&1 \
        | sed '/bashrc.*PS1/d';
    }

    make_target lint;
    make_target build-html;
    make_target build-pdf;
    make_target build-ps;
    make_target build-catman;
    make_target build-ex;
    make_target check;
    make_target dist;
)
ikerexxe commented 5 months ago

Ahh, so there's a way to see cmocka logs? I was quite frustrated in the past when cmocka reported a test failure because I didn't know how to see the logs, and had to manually reproduce the entire build inside a docker container and then cat the log file. :D

I think I never added cmocka logs to the reporting, so that kind of makes sense.

But, I don't think that's working as intended. See for example:

Something must have changed because it was working even when something failed. I'll take a look at it.

ikerexxe commented 5 months ago

Ok, so it seems it's not possible to get the logs when a command in docker build fails. I know there have been some changes around it with BuildKit, but maybe it never worked.

So, as I see it there are two options: either we move all the steps to scripts, or we move them to the Github workflow. Any other idea?

alejandro-colomar commented 5 months ago

Ok, so it seems it's not possible to get the logs when a command in docker build fails. I know there have been some changes around it with BuildKit, but maybe it never worked.

Except that we can do a trap(1) in the Dockerfile, since each RUN is basically a script.

So, as I see it there are two options: either we move all the steps to scripts, or we move them to the Github workflow. Any other idea?

I like stderr. If we can do both, then that's great. If we need to chose, I'd like to at least have stderr, which can be redirected to anything.

alejandro-colomar commented 5 months ago

v1b changes:

alejandro-colomar commented 5 months ago

It ain't the most beautiful thing in the world, but it works:

$ cat Dockerfile 
FROM debian
RUN bash -c "trap 'echo foo >&2' ERR; false;"
$ sudo docker build -f Dockerfile .
Sending build context to Docker daemon  28.67kB
Step 1/2 : FROM debian
 ---> 2a033a8c6371
Step 2/2 : RUN bash -c "trap 'echo foo >&2' ERR; false;"
 ---> Running in 262b18fe86de
foo
The command '/bin/sh -c bash -c "trap 'echo foo >&2' ERR; false;"' returned a non-zero code: 1
alejandro-colomar commented 5 months ago

v2 changes:

$ git range-diff shadow/master gh/trap trap 
1:  f9f8dc8e = 1:  f9f8dc8e .github/workflows/runner.yml: Use trap(1) to show the testsuite log
-:  -------- > 2:  eff32030 share/containers/: Specify one argument per line
-:  -------- > 3:  0dbd1b77 share/containers/: trap(1) to see the cmocka logs

I've also pushed those two commits to

to test them.

That run has already finished, and it looks good to me:

alejandro-colomar commented 5 months ago

I'll merge it already. It shouldn't be risky, given it only changes code running tests. And it might be useful to have this working in stable branches, in case we need to debug any problems. Thanks!

alejandro-colomar commented 5 months ago

v2b changes:

alejandro-colomar commented 5 months ago

v2c changes:

$ git range-diff shadow/master gh/trap trap 
1:  fe8caf4c ! 1:  eb5fcb4b .github/workflows/runner.yml: Use trap(1) to show the testsuite log
    @@ Metadata
     Author: Alejandro Colomar <alx@kernel.org>

      ## Commit message ##
    -    .github/workflows/runner.yml: Use trap(1) to show the testsuite log
    +    .github/workflows/runner.yml: trap(1) to see the testsuite log

         Otherwise, 'cat testsuite.log' isn't run, since 'set -e' aborts the
         script earlier.

    +    Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## .github/workflows/runner.yml ##
2:  ce2486f3 ! 2:  6c143c29 share/containers/: Specify one argument per line
    @@ Metadata
      ## Commit message ##
         share/containers/: Specify one argument per line

    +    Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
    +    Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>

      ## share/containers/alpine.dockerfile ##
3:  ef77b580 ! 3:  816b4fd7 share/containers/: trap(1) to see the cmocka logs
    @@ Metadata
      ## Commit message ##
         share/containers/: trap(1) to see the cmocka logs

    +    Reviewed-by: "Serge E. Hallyn" <serge@hallyn.com>
         Cc: Iker Pedrosa <ipedrosa@redhat.com>
         Signed-off-by: Alejandro Colomar <alx@kernel.org>