Closed perillo closed 5 months ago
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)?
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.
Some doubts about the code:
- In
utils.hh
some functions usesdoxygen
for documentation. Should I do the same for thelook_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!
I've merged the python2 packages for github actions in master, so it should now be possible to rebase this PR.
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).
Rebased to master, so that python2 is available.
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!
... and only one test fails now. Unfortunately, that one works fine for me, so I'm not quite sure what happens there.
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.
... 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.
... 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.
.... 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")
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.
I found where the problems are:
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).
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.
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 :warning: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
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.
Codecov Report
By the way, there are unit tests in the tests/unit-tests
directory. How do you run it?
The FreeBSD image has still problems:
Error: The process '/bin/bash' failed with exit code 64
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 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).
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
?
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.
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.
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!
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 thePATH
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.