Skip to content
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

out with autotools, in with cmake #9

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gquintard
Copy link
Member

it's the week-end, let's try something new!

I do realize that vcdk is the preferred method now, but having a lean repo just one git clone away is still very appealing to me.

This is mainly a test run for me to get accustomed to cmake, but I really liked what I saw and thought I should share. I opted against a dual autotools/cmake to show how much cruft we could remove, but I'm not against it.

@dridi, alternatively, that can be used to add a plugin to vcdk, feel free to run with it if you prefer that approach

@gquintard gquintard requested review from dridi and nigoroll August 22, 2021 06:29
@fwsGonzo
Copy link

fwsGonzo commented Aug 26, 2021

Hey! This looks great. I think a few things are still missing here, though. For example, it would have been nice to have a helper script with some functions that abstract away some of the boilerplate, like the creation of a VMOD shared library.

Additionally, tests need a few more properties, so they might also be better off as a function. For example, tests can have timeouts, skip return-codes, working directory and you can even mark a test as will-fail. Varnishtest currently uses the return code 77 for skipped tests.

Tests will have the name test01 which doesn't explain much, and it also prevents a bigger build system. Here we could do something like vmod_example_test_prints_hello from tests/prints_hello.vtc.

On that note I would also replace CMAKE_SOURCE_DIR with CMAKE_CURRENT_SOURCE_DIR, and I think that should be enough. I'm not sure if changing the binary dir also makes sense, but I guess that's just one of those things you will know at some point if you try to use this inside a bigger build system.

It is not currently possible to add VSCs.

Make sure that all the nice-to-have compiler flags are present: -fno-omit-frame-pointer so that the backtraces have better chances of working out-of-the-box. I think autotools adds _GNU_SOURCE, but I'm pretty sure that CMake doesn't!

Regardless, looks great :)
It definitely covers the basic VMOD example very well!

@gquintard
Copy link
Member Author

gquintard commented Aug 26, 2021

Thanks @fwsGonzo , that is awesome feedback, let's have a list to make sure we have everything:

  • regroup code in functions. Does it actually matter? the function would need to take the source file name as argument to be useful, which makes things complicated for little gain, apart for varnish-modules and varnish-cache if we ever convert them.
  • test using a function? did you mean that we'd have one call per test?
  • handle skipped tests
  • (optional) more options for test? I'm going to need help/pointers on that, I'm not sure how to accomplish it :-) the current autotools don't support it, do we care?
  • CMAKE_SOURCE_DIR -> CMAKE_CURRENT_SOURCE_DIR
  • rename test01.vtc (although I would argue that it could be done outside of this PR since it's not cmake related)
  • (very optionall) add VSCs support, I'd say we can cross that bridge once we reach it
  • compiler flags. Is there any cmake facilities to do this easily? (I mean, just copy what varnishd was built with, I don't see anything in pkg-config, and I believe it's a job for it)

That's quite the list, but feel free to pile on if you think some are missing. And of course, since you are the cmake expert here, feel free to add some commits in there (I can move the branch to this repo too if that makes it easier for you)

@gquintard
Copy link
Member Author

@fwsGonzo , could you have a look at 40e68d5 and see if that sort of matches your expectations please?

@fwsGonzo
Copy link

fwsGonzo commented Aug 30, 2021

I'll have a look when I find some time. It looks great. enable_testing() is a bit of a weirdo in that it only works when placed at the root level. So I'm not sure where to put it here. Maybe it's fine inside the function as vmod.cmake gets included, and will count as root? In a bigger build system the root CMakeLists takes care of that too.

@gquintard
Copy link
Member Author

@fwsGonzo , poke :-)

@fwsGonzo
Copy link

fwsGonzo commented Nov 7, 2021

Hey, thanks for the reminder! I've been finishing and moving into my new house the last few weeks and it's been very time consuming. I'll be having a go at this next week for sure!

@fwsGonzo
Copy link

From my PR at gquintard#1:

There are 2 things that may have to be done to make this the ultimate vmod.cmake:

  1. Detect that you are inside a larger build system and not use the system libvarnishapi. In that case you should use the libvarnishapi of the build system. This will allow things like fuzzing and sanitizing vmods and Varnish at the same time, per build folder. Very handy.
  2. Detect Varnish Plus and simply set VARNISH_PLUS in CMake as well as adding a -DVARNISH_PLUS to VMODs.

Something like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants