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

TOML config file support and migration #2221

Merged
merged 17 commits into from
Aug 23, 2019

Conversation

cryptocode
Copy link
Contributor

@cryptocode cryptocode commented Aug 16, 2019

This PR adds TOML support and migration of json config files on startup.

User facing changes:

  • The node and rpc binaries can operate without a config file as every setting has a default. The purpose of the config file is to allow these defaults to be overridden. The user only need to add the settings that are required instead of a complete file.
  • Config options can also be specified using a new --config option, which can be repeated multiple times. These options will take precedence over any entries in the config file (which doesn't need to exist). Example: nano_node --daemon --config rpc.enable=true
  • After migration, config.json is migrated into config-node.toml. Only non-default values are written to keep the config file small. Comments are added. rpc_config.json is migrated in a similar fashion.
  • The same config-node.toml file can be used for both the node and the Qt wallet.
  • After migration from json, config-node.toml and config-rpc.toml are never written to.
  • If migrating a Qt wallet config, config-qtwallet.toml will be created, containing wallet id and account. The account is updated if changed via the UI. If running the Qt wallet on a node directory, this file will be created.
  • Complete config files can be generated using --generate_config node|rpc. The user can use these directly, or pick what they need. Comments with schema information is added to facilitate future tooling/editor support. Sample output: https://gist.github.com/cryptocode/4d05774390518ddaadab61f838288d20
  • I've used the opportunity to clean up some settings during the migration. For instance, rpc_enable and opencl_enable are moved to enable under their respective config sections, to keep it consistent with other sections. The HTTP callback settings are grouped under a separate section.

Developer facing changes:

  • A git submodule update is required for building. I'm maintaining a fork of cpptoml that fixes bugs and improves TOML formatting, as well as adds features like key sorting and programmatically adding documentation. None of the upstream parsers supporting TOML 0.5 suffice.
  • JSON serialization is kept and there's a final upgrade path before migrating to TOML. This means the upgraded v20 JSON files on beta will work in the migration. After v20, no more upgrade paths are added and we simply add fields and defaults (as well as any validation code we want on the values.) At some point we can remove the JSON config code and require the user to upgrade via v2x first if they have json configs they wanna upgrade.

Should be ready for review, though some updates will be added:

We might extract more settings into separate toml files in upcoming PRs - the work generation part has been discussed, details remain.

@cryptocode cryptocode added documentation This item indicates the need for or supplies updated or expanded documentation beta testing wanted toml TOML related change labels Aug 16, 2019
@cryptocode cryptocode added this to the V20.0 milestone Aug 16, 2019
@cryptocode cryptocode self-assigned this Aug 16, 2019
nano/lib/CMakeLists.txt Outdated Show resolved Hide resolved
nano/lib/tomlconfig.hpp Outdated Show resolved Hide resolved
nano/lib/configbase.hpp Outdated Show resolved Hide resolved
nano/lib/tomlconfig.hpp Outdated Show resolved Hide resolved
nano/lib/tomlconfig.hpp Show resolved Hide resolved
nano/lib/tomlconfig.hpp Outdated Show resolved Hide resolved
nano/node/nodeconfig.cpp Outdated Show resolved Hide resolved
nano/node/nodeconfig.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@wezrule wezrule left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done some testing on Windows with node/rpc configs, working great. Also ran cppcheck which didn't find any warnings.

nano/lib/tomlconfig.hpp Outdated Show resolved Hide resolved
nano/node/node_rpc_config.cpp Outdated Show resolved Hide resolved
@zhyatt zhyatt requested review from guilhermelawless and removed request for SergiySW August 23, 2019 16:47
@cryptocode cryptocode merged commit 20b52ac into nanocurrency:master Aug 23, 2019
@cryptocode cryptocode deleted the feature/toml branch August 23, 2019 16:49
@zhyatt zhyatt added major This item indicates the need for or supplies a major or notable change and removed beta testing wanted labels Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This item indicates the need for or supplies updated or expanded documentation major This item indicates the need for or supplies a major or notable change toml TOML related change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants