NLnetLabs / simdzone

Fast and standards compliant DNS zone parser
BSD 3-Clause "New" or "Revised" License
68 stars 13 forks source link

We do not actually need to have cmocka installed, #49

Closed lemire closed 1 year ago

lemire commented 1 year ago

We can just build cmocka quickly on demand.

What this PR does is that if cmocka is not found, we just download it and build it.

This is how projects like CRoaring do it.

lemire commented 1 year ago

This is what I am getting...

$ ctest . --verbose
UpdateCTestConfiguration  from :/home/dlemire/CVS/github/simdzone/build/DartConfiguration.tcl
Parse Config file:/home/dlemire/CVS/github/simdzone/build/DartConfiguration.tcl
UpdateCTestConfiguration  from :/home/dlemire/CVS/github/simdzone/build/DartConfiguration.tcl
Parse Config file:/home/dlemire/CVS/github/simdzone/build/DartConfiguration.tcl
Test project /home/dlemire/CVS/github/simdzone/build
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 1
    Start 1: supported_types

1: Test command: /home/dlemire/CVS/github/simdzone/build/tests/zone-tests "-g" "types" "-t" "supported_types"
1: Test timeout computed to be: 0
1: [==========] Running 1 test(s).
1: [ RUN      ] supported_types
1: [       OK ] supported_types
1: [==========] 1 test(s) run.
1: [  PASSED  ] 1 test(s).
1/2 Test #1: supported_types ..................   Passed    0.00 sec
test 2
    Start 2: include_from_string

2: Test command: /home/dlemire/CVS/github/simdzone/build/tests/zone-tests "-g" "include" "-t" "include_from_string"
2: Test timeout computed to be: 0
2: [==========] Running 1 test(s).
2: [ RUN      ] include_from_string
2: <string>:1: $INCLUDE is not allowed
2: [       OK ] include_from_string
2: [==========] 1 test(s) run.
2: [  PASSED  ] 1 test(s).
2/2 Test #2: include_from_string ..............   Passed    0.00 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   0.00 sec

Is the <string>:1: $INCLUDE is not allowed something to be concerned with ?

lemire commented 1 year ago

Ping @k0ekk0ek

k0ekk0ek commented 1 year ago

Hi @lemire! Ah, yes. You're right cmocka is only required for testing. The error message is part of the test and intended, so no need to worry there. I figured users may want to have includes disabled when parsing strings for cases when it's used to parse non-trusted input and bad actors try malicious inputs and find a way to print contents or whatever.

As for downloading and installing, I chose to use Conan (C/C++ cross-platform package manager). That'll allow us to have Debug and Release builds for various platforms/compilers to be installed simultaneously. Doesn't mean we can't have this CMake script too, but you if you take a look at build-test.yml you might be pleasantly surprised at the ease of use(?) Still want to have this outlined in the README.md, so I should probably update the instructions.

Let me know, if the above works for you. I'll take a serious look at the CMake script if you still prefer that.

lemire commented 1 year ago

My PR does not break your approach, it is a fallback in case the user has not installed cmocka. I would argue, however, that for most users, it is a very reasonable default.

you might be pleasantly surprised at the ease of use(?)

I think that doing

cmake -B build -D BUILD_TESTING=ON
cmake --build build -j
ctest --test-dir build

... and having cmake figure out what to do and how to handle dependencies automatically is simplest. If cmocka is available, let it use it, if not, let cmake download and build it for you.

Let me know, if the above works for you. I'll take a serious look at the CMake script if you still prefer that.

It takes a few seconds to download and build cmocka along with simdzone. The script uses a download cache, so you only download cmocka once (you can have several build directories). Building cmocka only takes seconds... Here is how long it takes for me...

$ time (cmake -B build -D BUILD_TESTING=ON &&  cmake --build build -j && ctest --test-dir build)

real    0m7.295s
user    0m11.013s
sys 0m2.575s

So 10 seconds more or less... this includes building cmocka from scratch. Of course, you only need to build it once.

Installed dependencies are a failure point in general. For one thing, you ought to check that you have a proper version (my script actually nails the version down to the commit). And it works portably: the find function that the current code relies up fails on my macbook even though cmocka is installed system wide.

The beauty of my proposed approach is that it scales. E.g., if you need to add Google Benchmarks as a dependency, you don't also need to require that the user builds and install Google Benchmarks.

For some complicated dependencies, it will fail. For example, download and building ICU would be insane, it is too big or complex... but something like cmocka is tiny.

lemire commented 1 year ago

@k0ekk0ek So yes, I understand that you can install cmocka manually before building simdzone, but I invite you to consider that my PR makes the process smoother.

k0ekk0ek commented 1 year ago

Looking at this, there's two separate concerns(?)

  1. CMake cannot find the cmocka library installed on macOS.
  2. Choose between letting CMake build the library and/or have that handled by Conan.

The first may be caused by the library being installed through Homebrew in /usr/local/[Cellar|opt], or another location not searched by default(?)

The second is somewhat harder. Both give more-or-less the same benefits, tied to the branch, specific version, cache, etc. The single difference being:

conan install -s build_type=<build-type> -of . ../conanfile.txt

And that would install the required dependency tree.

But, I prefer it. To me the build system controls the build for the software developed by the project. It can try to be helpful by searching default locations, but IMHO it shouldn't control dependencies itself. At least, not download and build without user intervention. Conan downloads files too, but the step is purposely not integrated into CMake.

I like to fix the find functionality and leave the rest as is.

What is the system location on macOS? If we add that to HINTS in Findcmocka.cmake we should be ok?

k0ekk0ek commented 1 year ago

The linked PR at least fixes things for newer cmocka versions (the one that came with Homebrew on my system was 1.1.7).