Add feature to deprecate config options

[lanoxx]

We use libconfuse in tilda and often have the problem that we want to deprecate config options but this leads to problems:

When the config option is still present in the config file (e.g. for upgrading users) then libconfuse throws an error and we need to reset the config.

We are currently discussing if we replace libconfuse with another config library due to this, or if we are going to add a new feature to libconfuse. I would like to know if this project is still being actively maintained/developed and how likely it is that a patch will be accepted into libconfuse. If a patch would be accepted, then we might add a CFGF_DEPRECATED to the list of flags that causes libconfuse to ignore the config value and drop it when the config file is saved next. Possibly we could also split this into separate flags with a CFGF_DROP flag.

Please let us know what your plans with libconfuse are and if we can somehow help you.

[pik]

Hello, is libconfuse no longer maintained? Any chance to get an update on this?

[DavidEGrayson]

Based on recent conversations in this repo, it seems like @troglobit is actively maintaining the library and is excited about adding new features.

[troglobit]

Yep, that's right! 😃

However, even your new patch monkey has limited time on his hands. But I'm more than willing to handle pull requests for this issue. I may get around to looking at this myself, but that may take some time.

[troglobit]

@lanoxx Did you ever implement anything for this, or did you move away from using libConfuse altogether?

If you have anything, ever so small, I'm willing to take it upon myself to integrate it into the current master of libConfuse, just to ease the burden from you. (Quite a few changes since July ...)

Would love to hear back from you! 😃

[lanoxx]

I only opened this issue. There is no code yet. Maybe in the future I find some time to look at libconfuse code but its unlikely that will be before christmas.

[troglobit]

@lanoxx Ah, now I see .. completely missed this was referenced in lanoxx/tilda#161. I did not know about the __unknown feature in libConfuse, but I agree with @pik that declaring an option/setting with CFGF_DEPRECATED | CFGF_DROP looks a lot better.

I'll look into it, thank you for getting back! 😃

[lanoxx]

The unknown option was merged with this commit: 0255c50. On a side node I have to say that I am not too fond of that implementation. In my humble opinion I would prefer if libConfuse just ignored unknown options silently or if it could return an error struct with a list of unknown, invalid options or so, declaring this unknown token in every section seems a little too complicated and from the comments I guess that this does not work for lists.

Anyway back to this issue, support for a CFGF_DEPRECATED | CFGF_DROP option would be great. Let me know what you think after you have looked into it.

[troglobit]

We could definitely extend 0255c50 with more bells and whistles like you describe, when CFGF_IGNORE_UNKNOWN is given at cfg_init(), but I still think that the default behaviour should be as it is, to warn and fail.

However, I really like the idea of being able to deprecate settings using CFGF_DEPRECATED | CFGF_DROP! It may take me some time to get to work on it, since I'm occupied with tons of other things as well, so if anyone beats me to it with a pull request I'd be very happy 😃

[lanoxx]

After taking a short look at the code today, I think the best approach would be to extend the cfg_parse_internal function and implement the checks for CFGF_DROP there. If I understand the logic of the parser correctly then we have no state that tells us that parsing of an option has completed, instead we could add the code at the beginning of case 0:. At that point the opt pointer either points to null or to the last option that was successfully parsed. If opt is not null and both CFGF_DEPRECATED and CFGF_DROP are set, then we use cfg_free_value on the option.

If only CFGF_DEPRECATED is set then we should not free the option but instead just emit a warning.

Unfortunately this way we cannot catch the last line that being parsed, so we need to add the same code again at another place. I think there must be a nicer solution, so if someone has a suggestion let me know.

I gave it a quick shot and implemented some code, its not nice but it kinda works. You can take a look at the following gist: https://gist.github.com/lanoxx/4c5b13bf7097c13cbac5

[troglobit]

Interesting, thank you very much! 😃

I'll take a more detailed look at this later (probably tomorrow), integrate the patch, and see what I can do to extend it with what's missing (if anything!).

Awesome work! 👏

[lanoxx]

Its really just a quick hack, rather then something that I would merge. If we can find a way to add another state to the parser so we can catch when the parsing of an option line has finished and we are moving to the next option, then it would be easier and we would not need that duplicate code. Anyway, let me know what you think when you look at the code tomorrow.

[troglobit]

@lanoxx I've played with it now a bit, it's very promising, but I'd like us to agree on the semantics first. Something I think will result in a bit different implementation.

From my perspective I'd like to see the following definition:

• CFGF_DEPRECATED: for marking an option as still valid but may be removed in future releases of a software project
• CFGF_DROP: Can be set for any option to mark that it should be dropped by cfg_print*() functions

It should not be up to libConfuse to ignore/free deprecated settings, instead I'd like to leave that granularity up to the user so they can gradually phase out settings. Something like this:

1. First stage, warn that a setting is deprecated, internally migrate and recommend new setting, but still accept and save it. Gives backwards compatible .conf files for those who downgrade.
2. Second stage, still warn and accept but migrate to new setting and drop old setting on save. For handling upgrade from older configurations.
3. Third stage, warn, drop and ignore old setting. Useful for handling unsupported upgrade scenarios.
4. Final stage, with a new CFGF_IGNORE per-option flag, silently drop and ignore.

Hence, it would be up to the user of libConfuse to set up validator callback per deprecated option to decide what to do with it. Possibly we could add a new callback function for this, like for validators:

cfg_set_deprecated_func()

... or that could be used to simply be able to customize the deprecated warning string libConfuse would use for CFGF_DEPRECATED options. Dunno yet, but it's an outline of my ideas at least.

[lanoxx]

How about default callbacks and give the user the ability to override them?

[troglobit]

For CFGF_DEPRECATED options I'm leaning towards a very conservative default. I.e. we should just display a generic-deprecated-setting-warning. It would then be up to the user to provide an alternative generic warning text, e.g. using a cfg_set_deprecated_warning_string(cfg_t *cfg, char *string), or tailor an option-specific deprecated callback using cfg_set_deprecated_func().

Maybe what you really want is a per-option CFGF_IGNORE flag to pair with CFGF_DROP (and perhaps even pair it with CFGF_DEPRECATED :)?

[lanoxx]

Hi @troglobit,

I just saw your message.

For CFGF_DEPRECATED options I'm leaning towards a very conservative default. I.e. we should just display a generic-deprecated-setting-warning.

Isn't that exactly how I implemented it, except that right now there is no cfg_set_deprecated_warning_string function.

CFGF_DROP: Can be set for any option to mark that it should be dropped by cfg_print*() functions

I think I am already doing that in my patch, can you clarify what exactly you want to be done different?

I am not sure I understand what a CFGF_IGNORE flag would do? Can you clarify your preferred difference between CFGF_IGNORE and CFGF_DROP?

As I wrote when I posted my original patch, I had trouble integrating my changes. As you can see from my comment on Oct. 29th, I had some trouble with the parser states, could you suggest a better solution than how I attempted it? Especially regarding my comment:

If I understand the logic of the parser correctly then we have no state that tells us that parsing of an option has completed

If I missed such a state, the please let me know, otherwise could you maybe extend the parser to add such a state?

[troglobit]

@lanoxx You're absolutely right! I don't remember what I had been smoking that day ... 😃

I have no clue how to do this better. In pull request #53 I simply broke out the handling to a separate function. The rest of the PR is your code as-is plus some minor clean-up.

[troglobit]

Merged, closing issue.