configuration: fix incorrect handling of relative executables

[perillo]

Commit 720eb98 (configuration: refactorize code scanning for executables) introduced a bug when the executable is a relative path. In this case the file is always expanded, thus causing file to be always considered non executable by the code scanning through the parameters in configuration.cc.

Update the look_path function to first try a file without consulting the PATH environment variable.

The old code worked because the last argument (before the executable arguments) was update correctly thanks to the loop over the PATH environment variable.

Note that the correct solution is to add "--" before the executable arguments, but this will break the kcov cli interface.

This change should restore the original behavior.
What's left is to actually fix #414.

[perillo]

Some doubts about the code:

• In utils.hh some functions uses doxygen for documentation.
  Should I do the same for the look_path function?

• Is the performance the same as the original code (before my pull requests)?

[perillo]

ubuntu-latest does not include the python2 package.

@SimonKagstrom can you add it to the ci.yaml workflow, rebase this PR and run the tests again?

The CI tests also reported an error for python.python_issue_368_can_handle_symlink_target , my local tests also reported the same error.

If I remember correctly, this error was introduced with this change; when testing locally for commit 011ad7f this error did not occurred.

[SimonKagstrom]

Some doubts about the code:

• In utils.hh some functions uses doxygen for documentation.
  Should I do the same for the look_path function?

If you wish, but I haven't been strict at all with this!

• Is the performance the same as the original code (before my pull requests)?

I don't think it really matters: This is just done for parsing the arguments at startup, which should be very quick however it's implemented. The bulk of the processing time will anyway be in collecting coverage, so I think your implementation will be fine!

[SimonKagstrom]

I've merged the python2 packages for github actions in master, so it should now be possible to rebase this PR.

[perillo]

I've merged the python2 packages for github actions in master, so it should now be possible to rebase this PR.

Thanks.

But the workflow for FreeBSD is taking too much time (2h 33m). It is printing boot messages in a loop ...

I think it is better to add a shorter timeout to the ci.yml workflow (the default should be 6 hours).

[perillo]

Rebased to master, so that python2 is available.

[SimonKagstrom]

The FreeBSD support in github actions seems fickle, unfortunately. I don't use FreeBSD myself, so it's quite a hassle to fix it when it breaks. However, it at least has nothing to do with your change, so that should be mergeable anyhow!

[SimonKagstrom]

... and only one test fails now. Unfortunately, that one works fine for me, so I'm not quite sure what happens there.

[perillo]

https://github.com/SimonKagstrom/kcov/actions/runs/8221592577/job/22482125233?pr=419 is still running, even after I pushed a change. This seems a serious issue with the FreeBSD image, maybe it is better to send a bug report to Github.

[perillo]

... and only one test fails now. Unfortunately, that one works fine for me, so I'm not quite sure what happens there.

But why the test case name mentions i386? The CPU architecture should have nothing to do with symlinks.

[SimonKagstrom]

... and only one test fails now. Unfortunately, that one works fine for me, so I'm not quite sure what happens there.

But why the test case name mentions i386? The CPU architecture should have nothing to do with symlinks.

Hehe, almost but not quite :-)

It's issue_368, not 386, and it refers a regression test of Issue #368 in kcov.

I'll write a separate bug report for FreeBSD, although I don't think github themselves have anything to do with it.

[SimonKagstrom]

.... No, actually your last push was correctly built for FreeBSD. However, it also (now) lacks python2, so that needs to be installed. Alternatively, I guess we can make the Python2 tests Linux-only. That part should be the same on FreeBSD etc anyway.

Basically this change:

diff --git a/tests/tools/python.py b/tests/tools/python.py
index c223ad6..287ddb2 100644
--- a/tests/tools/python.py
+++ b/tests/tools/python.py
@@ -1,3 +1,4 @@
+import sys
 import testbase
 import unittest
 import parse_cobertura
@@ -25,6 +26,7 @@ class python_can_set_legal_parser(testbase.KcovTestCase):
         assert b"Cannot find Python parser 'python3'" not in o

 class python2_can_set_legal_parser(testbase.KcovTestCase):
+    @unittest.skipIf(not sys.platform.startswith("linux"), "Only for Linux")
     def runTest(self):
         self.setUp()
         rv,o = self.do(testbase.kcov + " --python-parser=python2 " + testbase.outbase + "/kcov " + testbase.sources + "/tests/python/main 5")
@@ -92,6 +94,7 @@ class python3_coverage(PythonBase):
         self.doTest("--python-parser=python3")

 class python2_coverage(PythonBase):
+    @unittest.skipIf(not sys.platform.startswith("linux"), "Only for Linux")
     def runTest(self):
         self.doTest("--python-parser=python2")

[perillo]

From https://github.com/SimonKagstrom/kcov/actions/runs/8221592577/job/22482125233?pr=419, you can see that the FreeBSD images hangs on "Run startVM". When I checked the log messages I found that the boot messages are printed, then boot starts again ... forever.

[perillo]

I found where the problems are:

1. The find_executable requires the file to be a regular file, so a symlink does not work. The original code worked thanks to the for loop over PATH.
   The fix is to simply check that it is not a directory (as it is done by Go).

2. I don't know why but the is_supported_executable function returns false for symlinks.

UPDATE

The culprit is peek_file: it uses lstat and then checks if it is a regular file.

lstat() is identical to stat(), except that if path is a symbolic link, then the link itself is stat-ed, not the file that it refers to.

[codecov]

Codecov Report

Attention: Patch coverage is 78.57143% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 65.81%. Comparing base (1d036f7) to head (b587483).
Report is 6 commits behind head on master.

Files	Patch %	Lines
src/utils.cc	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #419      +/-   ##
==========================================
+ Coverage   65.60%   65.81%   +0.21%     
==========================================
  Files          58       58              
  Lines        4504     4520      +16     
  Branches     4161     4177      +16     
==========================================
+ Hits         2955     2975      +20     
+ Misses       1549     1545       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[perillo]

This should really fix the implementation.

It was a good think that I decided to refactorize the old code; the code was wrong but it still worked correctly.

[perillo]

Codecov Report

By the way, there are unit tests in the tests/unit-tests directory. How do you run it?

[perillo]

The FreeBSD image has still problems:

Error: The process '/bin/bash' failed with exit code 64

[SimonKagstrom]

Very good that you found the error!

If you expand the commands in the FreeBSD logs, you can see the errors. It's the Python2 issue (plus a build issue in the tests, which should also link to curl).

I think that will be resolved by my Python2 patch above, which makes these tests Linux-only.

[SimonKagstrom]

The unittests have probably rotted away, but to build them, you should just run cmake against the tests/unit-tests directory (make sure you've done git submodule update first).

[perillo]

Very good that you found the error!

If you expand the commands in the FreeBSD logs, you can see the errors. It's the Python2 issue (plus a build issue in the tests, which should also link to curl).

I think that will be resolved by my Python2 patch above, which makes these tests Linux-only.

The only error I see is

Error: The process '/bin/bash' failed with exit code 64

after rsync in the "Copy files back from the VM" step.

I also found an interesting log message from rsync:

skipping non-regular file "kcov/kcov/tests/python/link_main"

Why does FreeBSD setup use rsync?

[SimonKagstrom]

Maybe only I can see it, but there's a > Run (run) [...] line in the output, which can be expanded. There one can see that the python2 tests fail.

I believe the FreeBSD virtual machine gets the code via rsync from the Mac it runs on, but I'm not quite sure how it's setup.

Anyway, if you're happy with the changes, I can merge your PR and fix the FreeBSD tests later, or else you could try adding my patch to disable python2 for it.

[perillo]

Anyway, if you're happy with the changes, I can merge your PR and fix the FreeBSD tests later, or else you could try adding my patch to disable python2 for it.

By "adding" do you mean amending the current commit or add a new commit?
In the former case, you can just add a new commit to this PR or commit your patch and rebase this PR.
In the latter case I think this is not a good idea, since the current commit already do two separate things (fix relative executable and fix symlinks).

UPDATE:

@SimonKagstrom
Why not install the python2 FreeBSD package?

prepare: pkg install -y binutils cmake elfutils python bash git

From a quick search, the python2 package should be available.

[SimonKagstrom]

Let's keep that as a separate issue. It really has nothing to do with your change.

I'll start with merging that, and resolve the FreeBSD problem in the other issue. May thanks for all the fixes!