search

zeek / cmake

CMake scripts used in Zeek
https://www.zeek.org
Other
48 stars 59 forks source link

zeek_add_plugin: No zeek_plugin_scripts() equivalent? #75

Closed awelzel closed 1 year ago

awelzel commented 1 year ago

I was looking at converting the init-plugin script's CMakeLists.txt to the new world order:

https://github.com/zeek/zeek-aux/blob/master/plugin-support/skeleton/CMakeLists.txt

It seems for zeek_plugin_scripts() there's no corresponding parameter for zeek_add_plugin() - was that on purpose?

zeek_plugin_scripts() was added not too long ago with fb332b85. Maybe I missed some discussion that this isn't wanted anymore, but potentially confuses users converting to the new approach what to do with existing zeek_plugin_scripts().

awelzel commented 1 year ago

@Neverlord - tagging you if you had any thoughts. Possibly ~very~ somewhat related to zeek/zeek#3107 in that we don't seem to honor dist files anymore either.

(edited - not necessarily related)

timwoj commented 1 year ago

Is it possible that this block from ZeekPluginDynamic is supposed to take care of that automatically?

https://github.com/zeek/cmake/blob/1b7be564b08a6071fca7277b285e76d64387facf/ZeekPluginDynamic.cmake#L100-L106

Neverlord commented 1 year ago

Is it possible that this block from ZeekPluginDynamic is supposed to take care of that automatically?

I believe so.

awelzel commented 1 year ago

Is it possible that this block from ZeekPluginDynamic is supposed to take care of that automatically?

I believe so.

The referenced commit fb332b8 introducing zeek_plugin_scripts motivation was that with a ./configure && make workflow, modifying any of the scripts within scripts did not automatically trigger rebuilding of the tarball.

With zeek_plugin_scripts() it was possible to tag scripts as dependencies for the tarball, so that the tarball was recreated. It would still package all of the scripts - #34 points out that this is maybe non-intuitive.

I think it makes sense to continue to have SCRIPT_FILES if only for symmetry with the previous approach. And also, in the future could potentially support another flag to zeek_add_plugin that disables the whole-sale scripts inclusion in favor of the explicit listed script files as pointed out in #34.

zeek_add_plugin(Namespace Plugin SCRIPT_FILES scripts/__load__.zeek NO_FULL_SCRIPTS_DIR)

I think I can manage adding SCRIPT_FILES to zeek_add_plugin().

EDIT: And given that the old zeek_plugin_end() now uses zeek_add_plugin() we did regress here as the old zeek_plugin_scripts helper is not having any effect anymore.