Update Meson build to 1.9.4 #1139
Conversation
|
cc @eli-schwartz for a review |
The meson build had gotten a little out of hand. It needed to be cleaned up and have its errors fixed. This should enable lz4 to switch to Meson at any time should the need ever arise.
|
It would be nice if unit tests could be added back too |
|
Where are the unit tests even defined in the Makefile build? I can't tell. |
|
hmm ok. I'll look into adding this. Seems like there are no arguments for the tests? Or I guess those are targets and I need to go look. Got it. |
|
Having automated CI tests for but I don't think they should be made part of What then matters is to have these tests running in CI. which is itself triggered by : The |
|
Also : could you write a high-level description in this PR head post of the shortcomings fixed by this PR ? For the time being, I've mostly noticed integration |
|
I will get to it. I wasn't going to add |
|
I could not get the fullbench and fuzzer tests to actually run, so I disabled them. Here is what is currently run. I could use help figuring out what other tests I could run. Reading makefiles is not my specialty. |
|
I added meson testing to the CI |
tristan957
changed the title
|
The freestanding test fails in debugoptimized and release builds under Meson: Seems like a potential stack overflow given the size of the struct. I don't really know how to debug this. |
Maybe the lack of |
|
I disabled the test in |
|
You could also set override_options for that test. |
Great point. I always forget this. I will update it tonight. Other than that, without anyone giving advice on what extra tests can and should be run, this PR seems to be nearing completion. I tried to get the tests working that use the argument Also, as you can see I create some static libraries for non-standalone fuzzers. Should those be exported as dependencies for external fuzzing tools to consume? It wasn't entirely obvious to me. |
|
@diizzyy what tests did you have in mind? |
| 'sources': files(lz4_source_root / 'tests/freestanding.c'), | ||
| 'c_args': ['-ffreestanding', '-Wno-unused-parameter', '-Wno-declaration-after-statement'], | ||
| 'link_args': ['-nostdlib'], | ||
| 'test': cc.get_id() in ['gcc', 'clang'] and get_option('optimization') not in ['2', '3'] |
There was a problem hiding this comment.
Again, this validation for the cc id is needed to detect whether you can build the executable at all... but "test" only determines whether after building it, it is defined as a test.
This either needs to be a "build" key, or pull it out of the dict and check the cc id, then append to exes +=
There was a problem hiding this comment.
hmm yea, didn't notice this. Will fix tonight.
There was a problem hiding this comment.
Would also be cool to get MSVC + meson testing here, athough the WrapDB will eventually test it after release.
There was a problem hiding this comment.
what is the msvc equivalent of the c and link args here?
There is no msvc testing in the CI currently, unfortunately.
|
@tristan957 |
|
I would not say having 4 tests is that good of coverage. |
|
True, but it's better than none. Of course, if we could get the same converage as using Makefiles working that would be great. |
|
The problem is identifying the tests. It isn't easy. |
|
The minimum set of tests that a built
Though if you also want to generate and then test the More advanced tests, including sanitizers and emulated targets, are already run using other build systems, I believe it's unnecessary to also build and run them through |
|
Ok I got test-lz4-skippable going. @Cyan4973 I moved the test out of the Makefile and into a POSIX shell script. Let me know what you think. If you like it, that is how I will continue writing the rest of the tests. The test script requires the |
|
I am having to do some shenanigans to not have to bump the Meson version unfortunately. |
tests/test-lz4-skippable.sh
Outdated
| SKIPFILE="goldenSamples/skip.bin" | ||
| FPREFIX="tmp-lsk" | ||
|
|
||
| "${LZ4}" -dc "$SKIPFILE" |
There was a problem hiding this comment.
Since this shell script depends on $LZ4 being defined,
a useful feature would be to start with a test checking if $LZ4 is defined
and output a human readable error if it's not.
There was a problem hiding this comment.
Yea, I was going to add that. Just became lazy :). I will get to it. Thanks.
I think it generally makes sense, now the devil is in the details, With regards to the test you extracted, I only have a minor comment, and I think it's good to go. The conversion method might be generic enough to be applied to other tests, |
|
If we convert test commands in
See also: https://stackoverflow.com/a/52961676 |
|
Ok. I was definitely going to add |
|
I would add |
|
Good point. |
More progress |
|
Is there any way to use Also does Windows even has a POSIX shell available to run the tests? |
|
My Makefile changes are obviously wrong. Could someone point out the issue and the correction? I presume it is related to the |
The It doesn't let you "test" the exit status of a command, but that's okay, bash already has that.
It's a common mistake that people think
You can install one, and similarly you can install tools such as diff, cmp, ls, cat, rm... You might need to check for their availability before running the tests, port to python, or include an additional Windows Batch version. |
|
I wasn't aware of starting a command with !. I thought that was make syntax. Good to know. I'll amend some of the tests tomorrow. |
|
test-lz4-basic is failing when converted to a meson test. Would appreciate help diagnosing. Is the test also broken in the Makefile build? |
No, it works fine. For example, see the following log. |
|
Somehow the two files differ only in the Meson build. Makes no sense to me |
I'd like to focus your first error in the log. https://github.com/lz4/lz4/runs/8263325959?check_suite_focus=true#step:15:33 (1) This error message means (2) I also couldn't find (3) But there's (4) Therefore, I think that invocation of (5) You may find similar errors. |
|
Yea I left off the .sh. I will come around to take a second look later today. |
|
@tristan957 |
|
I just haven't gotten back to this in a bit. Just got back from vacation. Maybe this week... |
|
ping @eli-schwartz too if you want to review |
tests/test-lz4-testmode.sh
Outdated
| echo "from underground..." > ${FPREFIX}2 | ||
| lz4 -dcfm ${FPREFIX}1 ${FPREFIX}2 | ||
| echo "---- non-existing source (must fail cleanly) ----" | ||
| ! lz4 file-does-not-exist |
There was a problem hiding this comment.
I think there is a problem in the translation of negative tests from Makefile to sh.
In the Makefile, if a test is expected to fail, it can be written, like here ! test_should_fail.
If, in contrast with expectation, test_should_fail actually succeeds,
the ! will make it fail, and Makefile execution will stop immediately.
However, for sh script, I don't think it works the same way.
I believe that, if test_should_fail succeeds, the ! will not stop execution.
As a consequence, the next line will be executed,
and the final result of the script will be unaffected by the outcome of test_should_fail.
This is obviously not a correct translation of a negative test.
When a test is supposed to fail, there is a need to proceed differently for sh. One method is to use the properties of && instead.
The test then becomes something like :
test_should_fail && die "the test should have failed"
with die() being a sh function designed to immediately stop execution of the script with an error code (typically exit).
So I'm afraid there's a bit more work to do.
There was a problem hiding this comment.
I appreciate the explanation. ShellCheck was giving me a similar warning, but I didn't quite understand it. With your explanation, I do get it. Fixed in latest update.
There was a problem hiding this comment.
Might be worth adding a ShellCheck GitHub Action to the CI. From what I can tell, all these shell scripts would pass, except for one.
There was a problem hiding this comment.
In a Makefile, the contents of a rule are evaluated as a series of shell scripts, each one run in turn but only if the previous one succeeded. The contents of the rules can be faithfully transcribed into a *.sh file, which @tristan957 was trying to do, and running set -e, the "errexit" option, causes the same rules to apply -- namely, that any command which fails causes the script to abort with an error, immediately.
Well, sort of. Actually, set -e has some intriguing gotchas. Some things aren't eligible to trigger early "error-exit". One of those things is... ! cmd. Another is cmd && othercmd, as well as cmd || othercmd.
Some gotchas of set -e are discussed at https://mywiki.wooledge.org/BashFAQ/105
Now, in the case of Make, since each line is actually executed as a script, you don't need errexit, because any line that fails will return that error to Make anyway. But for a shell script, you need to force any line that fails to exit explicitly. And that isn't as simple as it initially appears.
So yeah, that's why ! cmdcaused a problem here. Not because there's a difference between a Makefile and a shell script, but because the Makefile is running many small shell scripts, and the actual "real" shell script is just a single shell script.
There was a problem hiding this comment.
In a Makefile, the contents of a rule are evaluated as a series of shell scripts
This is good to know
There was a problem hiding this comment.
Might be worth adding a ShellCheck GitHub Action to the CI. From what I can tell, all these shell scripts would pass, except for one.
That's a good idea
There was a problem hiding this comment.
I could add a follow up PR after this one merges, assuming it will.
Specifically this adds support for the following options: - LZ4_ALIGN_TEST - LZ4_STATIC_LINKING_ONLY_DISABLE_MEMORY_ALLOCATION - LZ4_DISTANCE_MAX - LZ4_FAST_DEC_LOOP - LZ4_FORCE_SW_BITCOUNT - LZ4_FREESTANDING - LZ4_USER_MEMORY_FUNCTIONS - compiling ossfuzz targets - compiling more test targets - registering some tests
|
@Cyan4973 this is good to go. @eli-schwartz seems good to go with this as-is. A point release might also be nice, so Meson users can consume all the 1.9.4 goodies. |
|
I haven't taken a super close look at the latest version of this patch, but from a cursory inspection it seems good. The fact that the tests now run the same way in both Make and meson is convenient, and very maintainable. |
This takes advantage of a new upstream lz4 build system that I wrote in lz4/lz4#1139. Unfortunately I have to wait until 1.9.5 to see the fruits of my labor in Meson's WrapDB. Signed-off-by: Tristan Partin <tpartin@micron.com>
Specifically this adds support for the following options:
@Cyan4973 here are all the changes that are necessary to make the Meson build as featureful as the Makefile build. Please let me know if I have missed something. It would be nice to get a 1.9.5 release in the event this is merged so Meson users can get the benefits of the 1.9.4 release too. Thanks!