search

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

Debian packaging #444

Closed williamdes closed 1 month ago

williamdes commented 1 month ago

I noticed there is a debian/ folder in this repo (6 years old: https://github.com/SimonKagstrom/kcov/commits/master/debian) But the package https://tracker.debian.org/pkg/kcov Has a git repo in Debian Salsa: https://salsa.debian.org/debian/kcov Is there a reason for this folder here ?

I will have to contact Alessandro Ghedini and try to package the latest version

SimonKagstrom commented 1 month ago

It was a contribution due to the (back then) outdated debian package. I don't think it's relevant anymore, so good to remove!

It's a bit unfortunate that the debian packages are a bit behind, and thanks for taking it up with the maintainer!

williamdes commented 1 month ago

I got access to the Salsa repo, will be updating kcov in Debian.

Can you see if the patch in https://salsa.debian.org/debian/kcov/-/merge_requests/1/diffs can be merged upstream (here) ?

SimonKagstrom commented 1 month ago

Thanks!

I manually merged the patch from debian above.

Anyway, I suppose a release should be done before the update in Debian?

williamdes commented 1 month ago

Thanks!

I manually merged the patch from debian above.

Anyway, I suppose a release should be done before the update in Debian?

Yes, but I will probably trick the system to test that everything work before the release of v43

williamdes commented 1 month ago

Thank you so much for https://github.com/SimonKagstrom/kcov/commit/a7ab2971d377e6df84af52b748d46fa40d18f6c8

williamdes commented 1 month ago

Could you GPG sign your tags in the future ? And git commits too ?

williamdes commented 1 month ago

I think I need some help

On Debian I am trying to build tests. cmake ../tests/unit-tests

here is the error:

CMake Error at /usr/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find LibCRPCUT (missing: LIBCRPCUT_LIBRARIES
  LIBCRPCUT_INCLUDE_DIRS)
Call Stack (most recent call first):
  /usr/share/cmake-3.25/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  /mnt/Dev/@debian/@others/kcov/cmake/FindLibCRPCUT.cmake:54 (find_package_handle_standard_args)
  CMakeLists.txt:20 (find_package)

it looks like we have a package that has some files for trompeloeil: https://packages.debian.org/bookworm/amd64/libtrompeloeil-cpp-dev/filelist

This line should probably be removed: https://github.com/SimonKagstrom/kcov/blob/1e383e50ecd36eac142dc0f42698311466d9e645/cmake/FindLibCRPCUT.cmake#L46

And I do not find on internet where is the lib or file mentioned in https://github.com/SimonKagstrom/kcov/blob/1e383e50ecd36eac142dc0f42698311466d9e645/cmake/FindLibCRPCUT.cmake#L25

But https://github.com/rollbear/crpcut seems to be abandonned

williamdes commented 1 month ago

Also, it is not happy and reports a missing file: By not providing "FindElfutils.cmake" in CMAKE_MODULE_PATH

FindElfUtils.cmake has a different casing for u than find_package (Elfutils REQUIRED)

From: William Desportes <williamdes@wdes.fr>
Date: Mon, 15 Jul 2024 17:42:46 +0200
Subject: Fix casing of ElfUtils in CMakeLists

---
 tests/unit-tests/CMakeLists.txt | 2 +-
 tools/CMakeLists.txt            | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/unit-tests/CMakeLists.txt b/tests/unit-tests/CMakeLists.txt
index fa10574..647b0b7 100644
--- a/tests/unit-tests/CMakeLists.txt
+++ b/tests/unit-tests/CMakeLists.txt
@@ -19,7 +19,7 @@ endif (NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)

 find_package (LibCRPCUT REQUIRED)
 find_package (LibElf REQUIRED)
-find_package (Elfutils REQUIRED)
+find_package (ElfUtils REQUIRED)

 set (CMAKE_THREAD_PREFER_PTHREAD ON)
 find_package (Threads REQUIRED)
diff --git a/tools/CMakeLists.txt b/tools/CMakeLists.txt
index 93f79dd..3e79f11 100644
--- a/tools/CMakeLists.txt
+++ b/tools/CMakeLists.txt
@@ -11,7 +11,7 @@ option (SPECIFY_RPATH  "Specify RPATH for installed executables" OFF)
 mark_as_advanced (SPECIFY_RPATH)

 find_package (LibElf REQUIRED)
-find_package (Elfutils REQUIRED)
+find_package (ElfUtils REQUIRED)
 find_package (PkgConfig REQUIRED)
 find_package (Threads)
 find_package (ZLIB REQUIRED)
SimonKagstrom commented 1 month ago

Yeah, the unit tests would have to be revived. They have been disabled for quite a while, and I think the Debian package shouldn't build them either.

A lot of the code in kcov should really be rewritten. Not just the unit tests, but most of the codebase itself is based on old-style C++, and is not how I'd write things today. On the other hand, it's a lot of work for the same end result, and I also have other projects to work on.

Your patch looks good! But I think the best way is to not build the unit tests for Debian.

I'll have to readup a bit on the GPG-signed commits/tags.

williamdes commented 1 month ago

Your patch looks good! But I think the best way is to not build the unit tests for Debian.

Thanks, can you push it for me ? Thank you for https://github.com/SimonKagstrom/kcov/commit/8b020afff45d56f32b6538b0f3fea27bd9b08494 !

Yeah, the unit tests would have to be revived. They have been disabled for quite a while, and I think the Debian package shouldn't build them either.

Okay, I will remove the lines to build them

SimonKagstrom commented 1 month ago

Your patch looks good! But I think the best way is to not build the unit tests for Debian.

Thanks, can you push it for me ?

Done!

williamdes commented 1 month ago

Thank you !

On Debian I made most of the tests pass, only one remains: shared_library_accumulate. It boils down to one assert assert cobertura.hitsPerLine(dom, "solib.c", 10) == 1 The value is None Maybe I will just drop the line for now

SimonKagstrom commented 1 month ago

On Debian I made most of the tests pass, only one remains: shared_library_accumulate. It boils down to one assert assert cobertura.hitsPerLine(dom, "solib.c", 10) == 1 The value is None Maybe I will just drop the line for now

That's slightly strange, I'd say. I'll investigate that a bit in a branch to see if I can repair it.

SimonKagstrom commented 1 month ago

If you have time @williamdes , I've now pushed what might be an improvement to the test.

Still, I don't think it should have failed even before, but anyway.

williamdes commented 1 month ago

If you have time @williamdes , I've now pushed what might be an improvement to the test.

Still, I don't think it should have failed even before, but anyway.

Thank you, it passes on my workstation 🎉

But on the CI I still have something wrong to fix, not sure what: https://salsa.debian.org/debian/kcov/-/jobs/5985062#L1882

SimonKagstrom commented 1 month ago

Thank you, it passes on my workstation 🎉

But on the CI I still have something wrong to fix, not sure what: https://salsa.debian.org/debian/kcov/-/jobs/5985062#L1882

It looks like basically all compiled tests fail. I suspect this may be due to it being run under docker, i.e., the --security-opt stuff. I guess this might not be easy to change for the debian builder, so perhaps it's best/easiest to simply not run the tests?

williamdes commented 1 month ago

Thank you, it passes on my workstation 🎉 But on the CI I still have something wrong to fix, not sure what: https://salsa.debian.org/debian/kcov/-/jobs/5985062#L1882

It looks like basically all compiled tests fail. I suspect this may be due to it being run under docker, i.e., the --security-opt stuff. I guess this might not be easy to change for the debian builder, so perhaps it's best/easiest to simply not run the tests?

Indeed, again this bug :/ Is there no way for the tests/binary to auto detect the missing permission ?

The 32 bit test got the same result, for now I think the best is to skip such tests. If there was a way to detect the missing permission the tests would probably run fine on https://ci.debian.net/ IIRC it is not Docker based. Salsa CI is Docker based, I will search for security-opt

SimonKagstrom commented 1 month ago

Maybe one could add something like --no-ptrace to the test-suite runner. I can take a look at something like that.

I think the error will otherwise be some ptrace error message, but maybe that can be improved a bit. Won't be visible via the tests though.

williamdes commented 1 month ago

That would be great, let me know And if this can help with using kcov on systems without much privileges/hardened, then awesome

Let me know if you need some help

SimonKagstrom commented 1 month ago

It should run fine for bash/python even without the --security-opt stuff, but can't be used for compiled programs then.

I'll try to get the non-compiled tests to run for non-x86 archs as well, but we'll see how it goes!

SimonKagstrom commented 1 month ago

I've now merged a --no-ptrace option to master. It's used in the non-x86 Linux builds in ci.yml, and I hope something similar can be used in the Debian builds!

williamdes commented 1 month ago

Thank you !! It builds 🎉 https://salsa.debian.org/debian/kcov/-/jobs/5985292 and https://salsa.debian.org/debian/kcov/-/jobs/5985293

Let's see if there is more when I configure the autopkgtests

williamdes commented 1 month ago

Seems like https://ruderich.org/simon/blhc/ in unhappy https://salsa.debian.org/debian/kcov/-/jobs/5985301

SimonKagstrom commented 1 month ago

I don't quite see what it is unhappy about? If it's just about adding compile options, I'm fine with doing it

williamdes commented 1 month ago

I don't quite see what it is unhappy about? If it's just about adding compile options, I'm fine with doing it

Well, I am not sure either and will have to investigate about that tool. Something about compile logs hardening. Not problematic for a release or Debian package update

williamdes commented 3 weeks ago

I guess the next news I will post here is when v43 is uploaded to Debian, everything is green and ready for upload. Pending upload by an user with the power 🙏🏻 By the way, I liked the video you did about kcov: https://www.youtube.com/watch?v=1QMHbp5LUKg

SimonKagstrom commented 3 weeks ago

Great, thanks!

Looong talk - did some serious time miscalculation there.