Repository navigation
Conversation
3b2e88d to
7398f8a
Compare
|
One of the steps in the GitHub Workflow will post comments on the PR if it contains memory leaks. But it currently does not have permissions to do so. You'll need to grant those permissions to the repo. AI says:
|
|
Hey @sjoblomj - sorry for the late reply. I haven't had time to look at this PR yet. I would like to set up Valgrind locally and do some testing before I review - hope that is OK. |
fbb26de to
8a15946
Compare
|
@thomaslaurenson: I gave this PR some more attention, and I think it is leaner and cleaner than before. A comment will be automatically created in the PR when there are memory leaks. As I described in an earlier comment, you'd need to set up permissions for that to happen. As it stands now, the checks are failing because of lack of permissions, and because there are memory leaks in the code base. PR #173 fixes the latter. |
4c87163 to
6b6c0ee
Compare
|
@thomaslaurenson: I rebased and bumped the base image tag. The build is green now if you want to have a look :) |
The build tree moved to build/dev and the BUILD_MPQCLI option was renamed to MPQCLI_BUILD_APP. Also trigger the workflow on app/ and cmake/ changes.
pr.yml calls valgrind.yml as a reusable workflow, which requires an on: workflow_call trigger. Its own pull_request trigger is dropped so PRs run it once, filtered by pr.yml's paths and grouped by pr.yml's concurrency. The caller grants pull-requests: write for the PR comment, and the comment step is skipped on fork PRs, where the token is read-only.
The wrapper and the log analysis lived in three places, the workflow and the script's Docker and local modes, and had drifted apart. The workflow now installs Valgrind on the runner, like the test workflow builds there, and calls the script, which is the one place that does the work. The script: - only counts non-zero leaks, matching what the workflow checked, and also checks the error summary - empties valgrind_logs before a run, so old logs are not counted again - keeps going when tests fail under Valgrind, reporting them as a finding instead of exiting before the analysis - restores a binary left wrapped by an interrupted run, and restores it on Ctrl-C as well as on exit - always rebuilds as Debug through make, and uses the .venv that the Makefile creates instead of a separate test/venv - writes the GitHub step summary and status output itself Findings only warn: the script exits 0 on them. The job timeout goes up to 90 minutes, as the suite takes around 40 under Valgrind.
ubuntu:24.04 ships an "ubuntu" user holding UID/GID 1000, so creating the user failed for the most common host UID. It is now removed first. The work directory is also created owned by that user, as WORKDIR would make it root-owned and the build could not write into it. The image now only provides the tools and runs the script, which builds and tests the same way as outside Docker. apt-get update and install are in one layer, and sudo is gone.
ede182f to
f96b9db
Compare
This PR adds support for Valgrind, a memory leak detector.
The PR also adds tests for the
aboutandversionsubcommands. While those tests are pretty useless in themselves, they do complete the test suite for all subcommands, which also means Valgrind gets to check all subcommands.The PR adds
aboutandversionsubcommands.Convenience script
This will setup Valgrind in Docker, run the whole test suite and check for memory leaks in the code paths that the test suite covers:
Running a specific command
One can also invoke Valgrind for a specific command (in this case
mpqcli list wow-patch.mpq):(I created this PR using AI - it all seems sensible to me, but I am not an expert at either Valgrind nor GitHub Actions Workflows)