Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Debian packaging #444

Closed
williamdes opened this issue Jul 8, 2024 · 35 comments
Closed

Debian packaging #444

williamdes opened this issue Jul 8, 2024 · 35 comments

Comments

@williamdes
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

Thanks!

I manually merged the patch from debian above.

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

@williamdes
Copy link
Contributor Author

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
Copy link
Contributor Author

Thank you so much for a7ab297

@williamdes
Copy link
Contributor Author

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

@williamdes
Copy link
Contributor Author

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:

/home/ska/local/lib

And I do not find on internet where is the lib or file mentioned in

crpcut.hpp

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

@williamdes
Copy link
Contributor Author

williamdes commented Jul 15, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

williamdes commented Jul 15, 2024

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 8b020af !

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

@SimonKagstrom
Copy link
Owner

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Owner

Great, thanks!

Looong talk - did some serious time miscalculation there.

@williamdes
Copy link
Contributor Author

The version got uploaded 🎉
But for now there is two arches that report failure

/<<PKGBUILDDIR>>/src/solib-parser/lib.c: In function ‘force_breakpoint’:
/<<PKGBUILDDIR>>/src/solib-parser/lib.c:103:3: error: #error Unsupported architecture
  103 | # error Unsupported architecture
      |   ^~~~~
/<<PKGBUILDDIR>>/src/solib-parser/lib.c:105:25: error: expected string literal before ‘)’ token
  105 |                         );
      |                         ^
make[3]: *** [sr

@SimonKagstrom
Copy link
Owner

SimonKagstrom commented Sep 16, 2024

Great!

However, if possible I think the build should be restricted to x86, arm, and aarch64 which are really the only tested architectures. It might be possible (with changes to the build system) to build only for bash/python for other architectures.

Edit: loongarch and riscv can also be included. PPC in theory.

A long time ago, MacOS was supported this way, but this has been refactored away.

@williamdes
Copy link
Contributor Author

Okay
For now it seems like all checks are good https://tracker.debian.org/pkg/kcov
Is it important to remove the arches that are green on https://tracker.debian.org/pkg/kcov or not?
Let me know what is your opinion about having a strict list or only a list of the ones that pass tests.

@SimonKagstrom
Copy link
Owner

I don't think so. Many people use it for bash mostly anyway, and that should work anywhere. I personally only use it for compiled languages, but from the amount of bug reports, I seem to be in the minority :-)

So the conclusion is that I think it's fine as it is!

@williamdes
Copy link
Contributor Author

I don't think so. Many people use it for bash mostly anyway, and that should work anywhere. I personally only use it for compiled languages, but from the amount of bug reports, I seem to be in the minority :-)

So the conclusion is that I think it's fine as it is!

Okay, then what would be the patch to have s390x running? I will test and report that it works.
And mipsel64 even if I doubt there is some use of it

Like you said it should work for bash everywhere

@SimonKagstrom
Copy link
Owner

It would be in cmake and maybe main, to avoid registering the ptrace engine. Might be non-trivial though

@williamdes
Copy link
Contributor Author

This did come up https://qa.debian.org/bls/bytag/W-compiler-flags-hidden.html on https://qa.debian.org/bls/packages/k/kcov.html

Maybe there is come patch you can do to expose the compiler flags ?

@SimonKagstrom
Copy link
Owner

Not sure about that, actually. I've at least not done anything to attempt to hide the flags, except the normal cmake behavior. You'll need to build it with -v though:

[33/51] /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DPACKAGE -DPACKAGE_VERSION -I/Users/ska/projects/kcov/src/include -I/usr/local/opt/dwarfutils/include/libdwarf-0 -std=c++17 -g -Wall -D_GLIBCXX_USE_NANOSLEEP -DKCOV_LIBRARY_PREFIX=/tmp -DKCOV_HAS_LIBBFD=0 -DKCOV_LIBFD_DISASM_STYLED=0 -O3 -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.5.sdk -mmacosx-version-min=14.1 -MD -MT src/CMakeFiles/kcov.dir/writers/codecov-writer.cc.o -MF src/CMakeFiles/kcov.dir/writers/codecov-writer.cc.o.d -o src/CMakeFiles/kcov.dir/writers/codecov-writer.cc.o -c /Users/ska/projects/kcov/src/writers/codecov-writer.cc

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants