package-manager icon indicating copy to clipboard operation
package-manager copied to clipboard

Allow optional arguments for build_command

Open J-Gras opened this issue 3 years ago • 6 comments

So far the build_command of a plugin can only be customized using user_vars. However, user_vars enforce feedback of the user in all cases. Building the AF_Packet plugin for containers may serve as an example here: In many cases CMake logic can detect reasonable defaults and feedback is not required. Only if someone wants to build using a kernel different from the one of the building environment, manual configuration is required. Something like zkg install zeek-af_packet-plugin --build-params "--with-kernel=/usr/..." (or a more convenient version) would be nice to have I think.

J-Gras avatar Mar 14 '21 10:03 J-Gras

Would you need to modify zkg.meta in order to take advantage of the new variables? Or would the expectation be that zkg adds it automatically?

The issue I see with the generic build-params is that the common build command is something like ./configure && make, and it's not easy for zkg to pass the parameters in the right place. Do build params get passed to configure, to make, or to both?

I think that the current model would work if zkg.meta was updated to take advantage of env vars. Something like build_cmd=./configure $CONF_OPTS && make $MAKE_OPTS and then you run:

CONF_OPTS="--with-latest-kernel" zkg install zeek-af_packet-plugin, but that might be less user friendly.

grigorescu avatar Mar 14 '21 17:03 grigorescu

Would you need to modify zkg.meta in order to take advantage of the new variables? Or would the expectation be that zkg adds it automatically?

I would have expected that a change to zkg.meta as you described is required. Beyond that, I don't have a preference whether this should use some zkg-specific mechanism or env vars.

J-Gras avatar Mar 15 '21 08:03 J-Gras

We already have the %(XX)s syntax for user_vars, seems we could reuse that here as well. In fact why not just extend users vars so that they cover this as well? (I.e., make them optional, maybe allow to provide defaults from inside zkg.meta; and if we want env vars, add some way to preset a user var through the environment.).

rsmmr avatar Mar 15 '21 10:03 rsmmr

I'm having trouble understanding what exactly the problem is and what the suggestions are and how they're different from what can currently be done. The docs for user_vars at https://docs.zeek.org/projects/package-manager/en/stable/package.html#user-vars-field say:

  • %(XX)s syntax for user_vars substitution in zkg.meta should work, including in build_command
  • zkg.meta can specify suggested-default values for user_vars
  • user_vars won't prompt for feedback from user if:
    • using --force (default value gets used)
    • it's found in user's zkg config file (this also gets auto-populated on first response)
    • it's found as an environment variable of the same name

Then emphasizing that the build_command is "an arbitrary shell command" means you can write whatever custom script or branching/conditional logic that checks user_vars for empty/sentinel values that signify a default behavior (e.g. if it's an empty value don't add --with-kernel=/usr/...).

Given the above, is it currently possible to accomplish what's needed?

If it is, but it's just a bit annoying/confusing for users to respond to a prompt when most users can successfully rely on a default behavior, I can see adding a new feature to allow user_vars as specified in zkg.meta to mark certain ones as "never prompt, just use default value from zkg.meta unless they override from their config file or environment variable".

jsiwek avatar Mar 15 '21 20:03 jsiwek

I'm in the camp favoring that last suggestion above. User vars are a nice concept and the drawback atm is just inconvenience. I see two downsides when you use a general configure arg like this one:

  • When you don't want such a setting, you have to specify one anyway
  • When you have previously specified a value and now want to get rid of it, that's awkward: just hitting enter re-confirms the previous value, so you have to do something like enter a single space

If we add something to label user vars optional, I think we get what we need here.

Personally, I also think it would be nice if user vars were also exposed via command line options, since it seems useful to avoid the prompting, and the jump to environment variables is a bit far. That's why I like the idea of adding something like --user-var NAME=VAL. That might just be me, though.

ckreibich avatar Mar 15 '21 22:03 ckreibich

add something to label user vars optional, I think we get what we need here.

That's essentially what I meant as well.

Personally, I also think it would be nice if user vars were also exposed via command line options

Agree with this as a nice to have.

rsmmr avatar Mar 16 '21 07:03 rsmmr