ulfalizer / Kconfiglib

A flexible Python 2/3 Kconfig implementation and library
ISC License
450 stars 161 forks source link

Use None on write_autoconf for default header and add it as parameter on genconfig #80

Closed leandrolanzieri closed 4 years ago

leandrolanzieri commented 4 years ago

This PR modifies the write_autoconf function so that the default value for the header parameter is None, if that is the case then the default header title is added to the header file.

This allows things like the proposed modification on the genconfig script, where write_autoconf can be called with None when no specific header file title is needed. The behaviour of the function does not change when header is not specified.

ulfalizer commented 4 years ago

I wonder if this should be customizable via environment variables instead. Having to add separate options to each tool that writes configuration files and headers seems like a pain.

Could have e.g. KCONFIG_CONFIG_HEADER and KCONFIG_HEADER_HEADER, that would kick in if header=None (with an ultimate fallback on the default string). Thoughts?

cgundogan commented 4 years ago

Could have e.g. KCONFIG_CONFIG_HEADER and KCONFIG_HEADER_HEADER, that would kick in if header=None (with an ultimate fallback on the default string). Thoughts?

:+1: that would reduce command line clutter, escpecially for very long headers. However, being able to set the header explicitly via a command line option is also useful for setups, where modifying the environment is not that simple (testing, CI, ..).

What about supporting both options? With a preference of the command line option, if both are set? The environment variable would then represent the default header.

ulfalizer commented 4 years ago

Thanks for the changes!

Could have e.g. KCONFIG_CONFIG_HEADER and KCONFIG_HEADER_HEADER, that would kick in if header=None (with an ultimate fallback on the default string). Thoughts?

that would reduce command line clutter, escpecially for very long headers. However, being able to set the header explicitly via a command line option is also useful for setups, where modifying the environment is not that simple (testing, CI, ..).

What about supporting both options? With a preference of the command line option, if both are set? The environment variable would then represent the default header.

One issue with the command-line options is that I'd want all commands to support them, for consistency. Would need an allyesconfig.py --config-header="# foo bar" for example. That'd make all tools grow a bit, unless it could be handled with common code somehow.

I'll implement just the environment variable thing for now. I could maybe add some command-line stuff later on as well if it turns out to be messy though.

ulfalizer commented 4 years ago

I'll add this this weekend at the latest. Busy times with some devicetree stuff atm. :)

Gonna add KCONFIG_AUTOHEADER as another fallback for the header path at the same time. It's the name used by the C tools. The env. var. for the header preamble should probably be called KCONFIG_AUTOHEADER_PREAMBLE, for consistency.

leandrolanzieri commented 4 years ago

@ulfalizer I implemented the proposed solution. I assumed the default autoheader path to be the one used in genconfig.

leandrolanzieri commented 4 years ago
  • If --header-path is not passed, but KCONFIG_AUTOHEADER is set, then None is passed to write_autoconf() to use the path from the environment

The only downside that I see on this is that now the environmental variable has to be checked in genconfig.py as well.

ulfalizer commented 4 years ago

Added now, in https://github.com/ulfalizer/Kconfiglib/commit/de45874719772a40f1d8d244e2f5a6c6036415ac and https://github.com/ulfalizer/Kconfiglib/commit/bb3be6ee9793f23c275d318f58cb6cf21391b371. Will wait for feedback before pushing out a release.

I removed the Kconfiglib advertising, because it didn't feel worthwhile to add extra code and variables just to keep it. The default is now no header. :)

It's a bit gnarly that you have to add the trailing newline yourself in the common case of just adding a short comment at the top, though it has some nice side effects, like the header disappearing completely with KCONFIG_AUTOHEADER_HEADER="$FOO" when $FOO isn't set.

ulfalizer commented 4 years ago

Went with "header" instead of "preamble" for consistency with the parameter names. Hopefully it won't be confused with header as in header file.

leandrolanzieri commented 4 years ago

I removed the Kconfiglib advertising, because it didn't feel worthwhile to add extra code and variables just to keep it. The default is now no header. :)

Sounds good. @ulfalizer thanks for addressing this. Closing now