Introduce versioned config files and config_reader_min_version#697
Introduce versioned config files and config_reader_min_version#697fasterit merged 2 commits intohtop-dev:masterfrom
Conversation
f709162 to
b8eab12
Compare
|
I'm not sure if it is a good idea to embed a version number in the config file. Did anyone think of a good use case for this? IMHO, the config file should not be versioned, but htop should just ignore any unsupported fields. |
|
I don't think the exact htop version is of interest, but the parser version that produced this file is. |
|
May I ask why do we ever need to check the parser version? It suggests a backward incompatibility to the htop config format but I didn't see any backward incompatibility exist. And it wouldn't make sense to introduce a backward incompatibility to a config file anyway. This version checking PR (pull request) seems to be a solution in search of a problem. |
|
This PR is based on a problem we already face: One example of this is for example the active meters and columns in the configuration file. At the moment those are stored by referencing internal IDs that have to be kept equal at all costs for new versions of htop to be able to read old configuration files. This leads e.g. to the situation that some columns like CWD, EXE and COMM have IDs from the Linux-/platform-specific range for those IDs as those IDs have originally been implemented for Linux only and have recently been generalized (by PR #641 et.al.) to be platform-agnostic. The better way would be to have string identifiers for those and have htop map those to the correct (internal) IDs. This might be a minor inconvenience now, but with the introduction of dynamic meters and dynamic columns in PRs #707 et.al. this only worsens the issue: The internal ID now depends on the order that the config files for those columns are read on start-up. So the only way here is to actually store some name that can be resolved by htop when processing the config file. Also the old config format does not really allow to provide parameters to meters and columns, which is e.g. necessary for some meters like the Network I/O interface meter from #234. The idea behind this PR is not to magically make these new features work on old versions of htop (which wont work), but to allow old versions to detect this configuration downgrade. Also having this feature allows for updating the configuration in situations where field meanings subtly changed. For example the column IDs are stored off-by-one in current configuration files. With the version field available we could at some point decide that versions conforming to the new version would store those IDs as-as without that additional addition. That's also why I remarked that the config file should not only include the minimum version needed to process the configuration file, but also some current version to denote possibly non-breaking changes (like additional fields, that some older-version parser could ignore without too much hassle (except for dropping their values). |
|
@BenBE Thanks for information, but I'm personally not convinced. For most of the problem you mentioned, I think something like "feature flags" or "tags" would be a better approach. My idea is to let forks of htop benefit too - so that they can make local, incompatible changes to the config file without messing up with the "versioning". As an example, for the "column ID" off-by-one problem you mentioned, how about: storing the new IDs in the key name called |
|
Too complicated, @Explorer09 . htop will continue to ignore fields it doesn't know about. |
The reason why I put the htop version in is exactly that, we know parser or logic changes in between our versions. |
|
|
||
| typedef struct Settings_ { | ||
| char* filename; | ||
| int config_version; |
There was a problem hiding this comment.
This variable is never read from.
There was a problem hiding this comment.
It is set and made available when we have the next incompatible change to fence that one.
natoscott
left a comment
There was a problem hiding this comment.
Minor suggestions only, LGTM either way.
No description provided.