Installing scripts is broken for Spicy analyzers.

[J-Gras]

Looks like SPICY_SCRIPTS_OUTPUT_DIR_INSTALL is never set.

[bbannier]

This reproduces with the default template.

To expand on this, we install SCRIPTS registered with spicy_add_analyzer iff SPICY_SCRIPTS_OUTPUT_DIR_INSTALL is set,

cmake/ZeekSpicyAnalyzerSupport.cmake

Lines 75 to 84 in cc92336

	if (SPICY_SCRIPTS_OUTPUT_DIR_INSTALL AND DEFINED SPICY_ANALYZER_SCRIPTS)
	if (NOT DEFINED SPICY_ANALYZER_PACKAGE_NAME)
	message(FATAL_ERROR "SCRIPTS argument requires PACKAGE_NAME")
	endif ()
	install(
	FILES ${SPICY_ANALYZER_SCRIPTS}
	DESTINATION
	"${SPICY_SCRIPTS_OUTPUT_DIR_INSTALL}/${SPICY_ANALYZER_PACKAGE_NAME}/${NAME_LOWER}")
	endif ()

The original code in zeek/spicy-plugin populated this from spicyz --print-plugin-path, https://github.com/zeek/spicy-plugin/blob/a3e38215e7eb3ee733132c43cc3942379b038fc4/cmake/ZeekSpicyAnalyzerSupport.cmake#L161-L163

That flag now returns nothing for the version on the Zeek master branch, https://github.com/zeek/spicy-plugin/blob/a3e38215e7eb3ee733132c43cc3942379b038fc4/cmake/ZeekSpicyAnalyzerSupport.cmake#L161-L163

I am not really sure where the right location would be. Any ideas @rsmmr?

[bbannier]

The default package template for Spicy analyzers should set a script_dir in zkg.meta which makes analyzer Zeek scripts accessible at runtime, so this does not seem to be an issue. That being said, if one installs a Spicy analyzer directly from a manual CMake build, they will not be installed; this is similar to how manual installs of other Zeek plugin behave (also require manual copy).

I looked and the Spicy analyzers have behaved this way since the 5.x timeframe so the CMake code under that conditional has been dead code for some time.

[J-Gras]

this is similar to how manual installs of other Zeek plugin behave (also require manual copy).

I'm not following here. Doing a make install of a classic plugin copies its scripts to lib/zeek/plugins/<plugin_name> (tested using latest package template). Zeek 4.2 even introduced zeek_plugin_scripts() to trigger rebuilds on script changes.

Long story short: make install not resulting in a working installation is an issue that should be fixed. In case fiddling with scripts is out of scope, the install target should be removed altogether.

[awelzel]

I'm not following here. Doing a make install of a classic plugin copies its scripts to lib/zeek/plugins/<plugin_name>

Sorry, that was me adding to the confusion because I thought that didn't quite work with normal plugins either. But that was about a non-default scripts_dir in zkg.meta. The ./scripts dir is always installed - with some quirks around zkg interaction.

https://zeekorg.slack.com/archives/CSZBXF6TH/p1687910485598759?thread_ts=1687887010.223089&cid=CSZBXF6TH
amzn/zeek-plugin-s7comm@1737987

Long story short: make install not resulting in a working installation is an issue that should be fixed. In case fiddling with scripts is out of scope, the install target should be removed altogether.

Re-opening is the right call. Pinging @rsmmr again, if he had a preferred solution.

[rsmmr]

Iirc these used to go into a scripts directory associated with the plugin, which the plugin would automatically add to ZEEK_PATH so that they would be found. With the the plugin now part of Zeek, I''m thinking these should probably now just install into Zeek's site directory under the analyzer's name.

[ckreibich]

I'm circling back here since I just ended up in a support thread where this issue came up again and it still makes my head spin, plus I labeled this Zeek 7 above. :-)

I'm not sure I understand the use-case for the files specified in spicy_add_analyzer(SCRIPTS ...) — if in the past they were installed into the plugin's scripts folder, then they were subject to the plugin's load stage, so things like __preload__.zeek would work, which won't be the case if we start treating them like regular ZEEKPATH scripts. Also, if we now drop these into a folder below site named after the analyzer, then this risks collision (or at least confusion) with the directory zkg already creates for that purpose via symlinks into packages/<name>.

So I'm wondering — is there really a need for these scripts, or does the usual scripts_dir via zkg.meta cover the need? I am noticing that for built-in plugins spicy_add_analyzer() does not actually support SCRIPTS. So perhaps we just need to deprecate the use of SCRIPTS in the spicy_add_analyzer() version from ZeekSpicyAnalyzerSupport.cmake?

Fwiw, there are packages in the standard source (like spicy-http) that both use spicy_add_analyzer(SCRIPTS ...) in an analyzer directory while also pointing script_dir at this directory. This has the effect that unrelated files also get installed into what's supposed to be a tree of just Zeek scripts — things like CMakeLists.txt etc...

[awelzel]

So perhaps we just need to deprecate the use of SCRIPTS in the spicy_add_analyzer() version from ZeekSpicyAnalyzerSupport.cmake?

As discussed yesterday, to me this actually seems a valid approach, too. I've opened a PR to remove the rendering from the package-template as that seems confusing/misleading today given it's not working anyhow.

Emitting a deprecation warning if SCRIPTS are used (as it hasn't been functional) and advise users to leverage zkg and/or manually copying their scripts to the "right" place seems pragmatic.

For the latter, maybe a separate cmake function to install scripts into site/ could be interesting, leaving more decisions up to the user, but keeping it separate from spicy_add_analyzer() - I suspect that would most often collide with zkg logic if not used properly, but then that's not necessarily on us to fix.

[rsmmr]

As discussed yesterday, to me this actually seems a valid approach, too. I've opened a PR to remove the rendering from the package-template as that seems confusing/misleading today given it's not working anyhow.

Makes sense.

Emitting a deprecation warning if SCRIPTS are used (as it hasn't been functional) and advise users to leverage zkg and/or manually copying their scripts to the "right" place seems pragmatic.

Likewise agree.

For the latter, maybe a separate cmake function to install scripts into site/ could be interesting, leaving more decisions up to the user, but keeping it separate from spicy_add_analyzer() - I suspect that would most often collide with zkg logic if not used properly, but then that's not necessarily on us to fix.

Yeah, in principle people could just use normal CMake installation tools, but the trick is finding the destination path. Providing help with that would be good. Though the next question then is if the template should include such installation logic as well ...

[awelzel]

Yeah, in principle people could just use normal CMake installation tools, but the trick is finding the destination path. Providing help with that would be good. Though the next question then is if the template should include such installation logic as well ...

Yep, install(FILES ...) might just do, but I realize its then the decision of individual plugin/analyzer authors how scripts are installed within Zeek's tree - seems better to make this hard and mention zkg here.