Add plugin/CMakeLists.txt #37
Conversation
Support for BRO_PLUGIN_BASE has been removed, resulting in the
ZeekPluginDynamic.cmake code to pick-up ${CMAKE_CURRENT_SOURCE_DIR}/scripts
as the plugin's script. For the package-template's generated skeleton,
these are however the extra scripts usually installed via zkg.
Propose creation of another CMakeLists.txt within plugin/ to prevent
this.
The main wart here is that zeek-plugin-create-package.sh contains assumption
about the location of additional DIST_FILES (../ relative to the build directory),
so we copy these at configure time into build/ to fix this.
I'm liking this more than re-introducing support for BRO_PLUGIN_BASE,
but not sure it's overly great.
Closes #35
|
@bbannier - wdyt about this? |
| # Copy the dist files to package with the tarball into the top-level | ||
| # build directory to fulfill zeek-plugin-create-package.sh assumptions | ||
| # about them being located in the parent directory. | ||
| set(dist_files README CHANGES COPYING VERSION) | ||
| foreach(file ${dist_files}) | ||
| if ( EXISTS ../${file} ) | ||
| file(COPY ../${file} DESTINATION ${CMAKE_CURRENT_BINARY_DIR}/../) | ||
| endif() | ||
| endforeach () |
There was a problem hiding this comment.
Like I mentioned offline, not a fan of the magic modifications to the "file names" since this makes it pretty hard to modify for users, what if they want to e.g., include a generated file in the tarball? I'd prefer just directly referencing files with relative paths and requiring them to exist; in addition to the technical implementation comment we also need a user-targeted comment explaining what this variable is for -- for anything else we use magic globs 🤢.
Probably worse, I just noticed that packaging already copies some files around, https://github.com/zeek/cmake/blob/2df3b8e82a843b7b8187963d259d32a9fb42b873/ConfigurePackaging.cmake#L133-L140, i.e., we would already create a README.txt and COPYING.txt in the build directory. This could potentially clash with what users set up here (e.g., if they named their README as README.txt). I do not think there is a good automagic way to fix this, can we augment zeek-plugin-create-package.sh so it can understand explicit dist files?
There was a problem hiding this comment.
can we augment zeek-plugin-create-package.sh so it can understand explicit dist files?
Not sure I fully understand how to do this, but agree the file copying isn't great - it just attempts to please zeek-plugin-create-package.sh. Plainly using .../VERSION currently fails as the create package script then copies it not into dist, but dist/../VERSION. Hmm, we could possibly just strip the leading ../'s before determining the destination directory within ./dist, that could actually be "okay".
There was a problem hiding this comment.
We could also just have the semantics that the files end up in the same place they were relative to the CMakeLists.txt with project here, e.g., from a subdirectory ../README (which is the same as <project>/README) gets copied <dist>/README. I left a similar comment on zeek/cmake#116.
Support for BRO_PLUGIN_BASE has been removed, resulting in the ZeekPluginDynamic.cmake code to pick-up ${CMAKE_CURRENT_SOURCE_DIR}/scripts as the plugin's script. For the package-template's generated skeleton, these are however the extra scripts usually installed via zkg.
Propose creation of another CMakeLists.txt within plugin/ to prevent this.
The main wart here is that zeek-plugin-create-package.sh contains assumption about the location of additional DIST_FILES (../ relative to the build directory), so we copy these at configure time into build/ to fix this.
I'm liking this more than re-introducing support for BRO_PLUGIN_BASE, but not sure it's overly great.
Closes #35