SimonKagstrom / kcov

Code coverage tool for compiled programs, Python and Bash which uses debugging information to collect and report data without special compilation options
http://simonkagstrom.github.io/kcov/
GNU General Public License v2.0
709 stars 109 forks source link

CI upgrades and docker setup #440

Closed williamdes closed 1 month ago

williamdes commented 1 month ago

This is marked as draft but you can merge my first CI commits

The action https://github.com/dawidd6/action-get-tag is deprecated, I replaced it with logic I use at work.

SimonKagstrom commented 1 month ago

Looks good to me!

Perhaps the docker image would be better to switch to alpine while we're at it? At least my Raspberry pi images built on alpine tends to be smaller than the debian-based ones.

williamdes commented 1 month ago

It looks like s390x does not build

  > [linux/s390x builder 4/4] RUN mkdir /src/build &&     cd /src/build &&     cmake -G 'Ninja' .. &&     cmake --build . &&     cmake --build . --target install:
20.40       |   ^~~~~
20.40 /src/src/solib-parser/lib.c:105:25: error: expected string literal before ')' token
20.40   105 |                         );

https://github.com/williamdes/kcov/actions/runs/9819307965/job/27112899974#step:6:10565

SimonKagstrom commented 1 month ago

Didn't know it tried to build for s390!

Anyway, kcov only supports x86, arm, aarch64/arm64, riscv and loongarch, so no need to build for s390 etc.

SimonKagstrom commented 1 month ago

And quite frankly, Mips and PowerPC probably hasn't been used for years, so I think ARM, aarch64 and x86 is enough.

The error is also slightly misleading. I would have expected an "#error Unsupported architecture" to trigger.

williamdes commented 1 month ago

Didn't know it tried to build for s390!

It's not yet on my PR

Anyway, kcov only supports x86, arm, aarch64/arm64, riscv and loongarch, so no need to build for s390 etc.

Well, I have a s390x VM at IBM hosted by Linux One. It would be nice to have it supported.

But yeah, let's drop some old archs. Even if this hurts me a bit but they are probably not used and wasting power to build them is no good.

It's

SimonKagstrom commented 1 month ago

They are still built and tested (well, not MIPS), just not the docker image.

As for s390, it needs a few lines for the breakpoint encoding. Probably not difficult to add, but I don't really have a good way of testing it myself (and no personal use for it either).

williamdes commented 1 month ago

They are still built and tested (well, not MIPS), just not the docker image.

As for s390, it needs a few lines for the breakpoint encoding. Probably not difficult to add, but I don't really have a good way of testing it myself (and no personal use for it either).

Do you want me to add your ssh key to my Debian 12 VM? Email me at williamdes / wdes.fr

SimonKagstrom commented 1 month ago

As for s390, it needs a few lines for the breakpoint encoding. Probably not difficult to add, but I don't really have a good way of testing it myself (and no personal use for it either).

Do you want me to add your ssh key to my Debian 12 VM? Email me at williamdes / wdes.fr

Thanks for the offer, but since I won't really use it myself it would be better for someone else to implement and add it. Also, I guess the s390 architecture might be special in other ways as well, so maybe it's not as the others, which are basically

[...]
#elif defined(__powerpc__)
    val = 0x7fe00008; /* tw */
#elif defined(__arm__)
    val = 0xfedeffe7; // Undefined insn
[...]

(and slightly more involved for x86 etc).

williamdes commented 1 month ago

Same error in mips64le: https://github.com/SimonKagstrom/kcov/pull/440#issuecomment-2211818081 Logs: https://github.com/williamdes/kcov/actions/runs/9821201170/job/27116829723#step:6:9314

I am testing all archs supported by the Debian image and commenting out the unsupported ones. One by one. So we have a list at today's date.

SimonKagstrom commented 1 month ago

Great, thanks for all the effort!

williamdes commented 1 month ago

Build success of: linux/386,linux/amd64,linux/arm/v5,linux/arm/v7,linux/arm64/v8,linux/ppc64le See: https://github.com/williamdes/kcov/actions/runs/9821655706/job/27117558504#step:6:9856

In my last commit I removed ppc64le as you requested (https://github.com/SimonKagstrom/kcov/pull/440/commits/0b06d0068b1f45c29a210b2d73056270b3214a59)

williamdes commented 1 month ago

Perhaps the docker image would be better to switch to alpine while we're at it? At least my Raspberry pi images built on alpine tends to be smaller than the debian-based ones.

Could we merge and test all this before switching to Alpine ?

williamdes commented 1 month ago

I added an "environment" name, it has some benefits:

You are not obliged to move the existing secrets into it, no change is required at all. But when this PR is merged, feel free to put the secrets into an env and maybe use a token and not your password in the password env.

image

SimonKagstrom commented 1 month ago

Perhaps the docker image would be better to switch to alpine while we're at it? At least my Raspberry pi images built on alpine tends to be smaller than the debian-based ones.

Could we merge and test all this before switching to Alpine ?

Absolutely!

I'll take a look at this tomorrow though, but thanks a lot again for the effort!

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 65.31%. Comparing base (f3edbf6) to head (0b06d00). Report is 2 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #440 +/- ## ========================================== - Coverage 65.68% 65.31% -0.37% ========================================== Files 58 58 Lines 4514 4541 +27 Branches 4171 4198 +27 ========================================== + Hits 2965 2966 +1 - Misses 1549 1575 +26 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

williamdes commented 1 month ago

It looks like I can not build the current code on Alpine

#5 1.900 /src/src/engines/ptrace_linux.cc: In function 'long int getRegs(pid_t, void*, void*, size_t)':
#5 1.900 /src/src/engines/ptrace_linux.cc:332:24: error: '__ptrace_request' was not declared in this scope
#5 1.900   332 |         return ptrace((__ptrace_request ) PTRACE_GETREGS, pid, NULL, regs);
#5 1.900       |                        ^~~~~~~~~~~~~~~~
#5 1.900 /src/src/engines/ptrace_linux.cc: In function 'long unsigned int ptrace_sys::peekWord(pid_t, long unsigned int)':
#5 1.900 /src/src/engines/ptrace_linux.cc:364:24: error: '__ptrace_request' was not declared in this scope
#5 1.900   364 |         return ptrace((__ptrace_request ) PTRACE_PEEKTEXT, pid, aligned_addr, 0);
#5 1.900       |                        ^~~~~~~~~~~~~~~~
#5 1.900 /src/src/engines/ptrace_linux.cc: In function 'void ptrace_sys::pokeWord(pid_t, long unsigned int, long unsigned int)':
#5 1.900 /src/src/engines/ptrace_linux.cc:369:17: error: '__ptrace_request' was not declared in this scope
#5 1.900   369 |         ptrace((__ptrace_request ) PTRACE_POKETEXT, pid, aligned_addr, value);
#5 1.900       |                 ^~~~~~~~~~~~~~~~
#5 1.900 /src/src/engines/ptrace_linux.cc: In function 'long int setRegs(pid_t, void*, void*, size_t)':
#5 1.900 /src/src/engines/ptrace_linux.cc:380:24: error: '__ptrace_request' was not declared in this scope
#5 1.900   380 |         return ptrace((__ptrace_request ) PTRACE_SETREGS, pid, NULL, regs);
#5 1.900       |                        ^~~~~~~~~~~~~~~~

It looks like this is something to fix in the code ?

williamdes commented 1 month ago

In fact this is needed CXXFLAGS="-D__ptrace_request=int" cmake -G 'Ninja' .. Not sure maybe the code could be adjusted to avoid this ?

SimonKagstrom commented 1 month ago

Yes, I think the code should be adjusted instead. Not actually sure the cast is needed at all. The FreeBSD port doesn't use it, for example.

So a first test could be to simply remove all of (__ptrace_request).