.github: add initial workflow to run tests

ee7 commented 1 month ago

Add an initial workflow to run dev run in CI on the Linux and macOS runners. The output currently needs to be inspected manually, but this PR gives us a base to build on and could already help catch regressions like #37 (and make it easier to verify fixes on other platforms).

Note that currently:

The dev run command exiting with an exit status of 0 does not imply that the tests pass.
The workflow indicates success even when dev run segfaults.

We can improve such things over time.

Also use the latest stable GCC release (GCC 14.1) in the Linux job to help make newer compiler warnings more visible.

Closes: #34 (we can make follow-up tickets for e.g. the exit status) Refs: #35 Refs: #37

To-do:

[x] Fix error on macOS: meson.build:170:14: ERROR: C shared or static library 'crypto' not found
[x] Resolve any conflict with main after we merge a PR that fixes #37

miki725 commented 3 weeks ago

https://github.com/crashappsec/libcon4m/blob/833300442d95e6ca7b49104417873c0c671caa00/dev#L128-L139

looks like exit code is not honored in the dev script in general. not sure how that works on mac at all. we can either set -e in the script to bail out on any non-successful commands or explicitly handle the case when ./c4test fails

ee7 commented 3 weeks ago

Yes, my intention for this PR was simply to get dev run into CI. It's true that alone currently isn't as useful as it should be, because the dev script has some problems. But let's handle that in a separate PR, which should make dev run exit non-zero when c4test exits non-zero. That will fix the behavior of this workflow without any changes to the workflow itself.

I would've already added set -e to the script, but we can't do that alone because e.g. we currently have this typo:

https://github.com/crashappsec/libcon4m/blob/833300442d95e6ca7b49104417873c0c671caa00/dev#L133

and changing mason to meson won't work because meson_build always exits the script:

https://github.com/crashappsec/libcon4m/blob/833300442d95e6ca7b49104417873c0c671caa00/dev#L66-L73

so just adding set -e and fixing the typo means that dev run will never run c4test.

This is what I meant when I mentioned the typo yesterday, and with:

Note that currently:

[...]

The workflow indicates success even when dev run segfaults.

[...]

Closes: https://github.com/crashappsec/libcon4m/issues/34 (we can make follow-up tickets for e.g. the exit status)

viega commented 3 weeks ago

I don't understand the need to be rushing it in if it isn't where it needs to be. There's no need to overengineer the build; I don't want to do more than the bare minimum necessary (for instance with the dev script), because they're not a priority yet. Breaking this up feels like an invitation to bikeshed, and I would rather stay focused.

ee7 commented 3 weeks ago

I don't understand the need to be rushing it in if it isn't where it needs to be

It's just that:

I prefer PRs to be tightly focused.
I considered adding our first workflow file, and altering the behavior of dev run, to be issues that can be considered independently.
It already seemed handy, given that c4test on main currently produces segfaults that you don't reproduce locally.

But fair enough if we don't want to have, even for a momentary first implementation, a workflow that indicates success when the tests clearly error.

Breaking this up feels like an invitation to bikeshed

I can change dev in this PR (and have done so with https://github.com/crashappsec/libcon4m/pull/38/commits/4729312a3d99c478a0530953b27de0edba0bf33f), but doing it here means that any objection to those changes blocks this PR rather than another one. But it sounds like this PR was blocked anyway.

For instance, it's certainly not working on Linux right now based on the runner output, but it still passes.

Also, since I'm generally not working on a Linux box and switching is a bit of a PITA, would appreciate some insight there.

I've opened https://github.com/crashappsec/libcon4m/issues/41 including a stack trace and CON4M_DEV debug output, ~and set the CON4M_DEV env var in the workflow in this PR~. Does that help?

ee7 commented 3 weeks ago

Just to document the current state in the latest Linux CI runs (see log): the segfaults are for basic11 and basic18:

[-- libcon4m --] Running test: [== basic11.c4m ==]
./dev: line 143:  3776 Segmentation fault      (core dumped) ${HERE}/c4test $ITEM

[-- libcon4m --] Running test: [== basic18.c4m ==]
./dev: line 143:  3866 Segmentation fault      (core dumped) ${HERE}/c4test $ITEM

and the other failures are for basic14, basic15, and basic16:

FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic14.c4m: output mismatch.
Expected output
12
12
7
7
Actual
12
12
12
12

FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic15.c4m: output mismatch.
Expected output
4
Actual
0

[-- libcon4m --] Running test: [== basic16.c4m ==]
Runtime Error: length cannot be < 0 for string initialization
Stack Trace:
    module basic16:15
info: Compiling: /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m
info: Done processing: /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m
****STARTING PROGRAM EXECUTION*****
****PROGRAM EXECUTION FINISHED*****

FAIL: test /home/runner/work/libcon4m/libcon4m/tests/basic16.c4m: program expected output but did not compile. Expected output:
 Hello,

ee7 commented 3 weeks ago

This was approved by https://github.com/crashappsec/libcon4m/pull/49#issue-2377247517. Merging.

crashappsec / libcon4m

.github: add initial workflow to run tests #38