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

Allow optional arguments for build_command #98

Open
J-Gras opened this issue Mar 14, 2021 · 6 comments
Open

Allow optional arguments for build_command #98

J-Gras opened this issue Mar 14, 2021 · 6 comments

Comments

@J-Gras
Copy link
Contributor

J-Gras commented Mar 14, 2021

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.

@grigorescu
Copy link
Contributor

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.

@J-Gras
Copy link
Contributor Author

J-Gras commented Mar 15, 2021

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.

@rsmmr
Copy link
Member

rsmmr commented Mar 15, 2021

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.).

@jsiwek
Copy link
Contributor

jsiwek commented Mar 15, 2021

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".

@ckreibich
Copy link
Member

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.

@rsmmr
Copy link
Member

rsmmr commented Mar 16, 2021

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.

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

No branches or pull requests

5 participants