-
-
Notifications
You must be signed in to change notification settings - Fork 111
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
Build multi OS for Docker #452
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #452 +/- ##
==========================================
- Coverage 69.85% 69.60% -0.26%
==========================================
Files 55 55
Lines 4249 4250 +1
Branches 3906 3909 +3
==========================================
- Hits 2968 2958 -10
- Misses 1281 1292 +11 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
Man, you work so quickly! :-)
OK, so the fork-32 test fail to build (it appears, from the logs). The test was not really working anyway, so I removed it in master. Perhaps you could try to merge/rebase onto that?
Yeah, I did! The git tag is what mostly determines the version. This is for some cmakery that another guy contributed, so I'm not completely sure why it's needed. But feel free to update it! |
Haha, 10 years of work ;p
Done
Maybe we should remove the Cmakelist stuff and determine the version from that constant(s) ? |
It looks like I have the same result as when I tried to make tests work into the Docker image No tests seem to pass, not sure why |
Might be several reasons, I think. For one, it seems like maybe /sbin is not part of the PATH:
but my guess is that this is caused by the alpine image not being run with Anyway, I'm OK with just building it under alpine, so if there's no easy fix, then I think we should just disable the tests there (like non-x86:es). |
I can take a look at that. I guess the version should be settable from the git tag for starters at least. |
Thank you for the last commit, is there a way that I can call the function and get the output ? |
Well, my commit actually didn't build on github (yes, I should have used a branch first...). I think it's due to missing tags, so I'll attempt to repair it before reverting it. I'm not quite sure I understand what you want? You mean the version? If you have built kcov, you can get it via
|
Same as me today, see ad89c0f You need to have fetch depth 0 or some other depth or maybe just try to enable fetch tags ?
Indeed, but is there a way to get it before the build as I need to inject the version built into the metadata before launching docker |
The latest master should build again. I guess it might be possible to replicate in ci.yml? It's really just boils down to |
Yay, can you try only switching on the fetch-tags with a fetch depth of 100 maybe ?
Not sure what you mean here |
With coreutils-env
@@ -26,7 +26,7 @@ def runTest(self): | |||
class lookup_binary_in_path(libkcov.TestCase): | |||
@unittest.expectedFailure | |||
def runTest(self): | |||
os.environ["PATH"] += self.sources + "/tests/python" | |||
os.environ["PATH"] += os.pathsep + self.sources + "/tests/python" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was the patch needed to fix things up
/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:./tests/python
without the separator nothing works correctly, and rm, mv and friends can not be found
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, good catch!
I wonder why it was like that? I guess maybe self.sources was an absolute path in some cases, and a relative one in another. Anyhow, good that it's fixed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best guess the last path in the list was not that important, but not on Alpine haha
Always add the separator 😄
container: | ||
image: alpine:3.20 | ||
# Needed to attach to a binary | ||
options: --security-opt seccomp=unconfined |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, as you suggested, add this to the instructions somewhere? I guess the docker info can be placed in a doc/docker.md
file, to reduce the clutter in README.md.
|
It should be okay now, I updated the platform list for Debian. They do not have riscv64 |
kcov needs access the following system calls: | ||
- [`ptrace`](https://linux.die.net/man/2/ptrace) | ||
- [`process_vm_readv`](https://linux.die.net/man/2/process_vm_readv)/[`process_vm_writev`](https://linux.die.net/man/2/process_vm_writev) | ||
- [`personality`](https://linux.die.net/man/2/personality) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well written with references!
Merged, thanks a lot! |
Currently waiting on the Docker build to fail push (workflow dispatch is very cool !) after build success: https://github.com/williamdes/kcov/actions/runs/9929723485 |
Thank you too for this very nice and active collaboration ! |
I'm sure it will build fie! I saw the workflow_dispatch stuff, something to read up on in the future! |
Something I am a bit unsure about is why it looks like the docker build also triggers for PRs, maybe a side effect of workflow dispatch |
Fixes #451
Fixes #450