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.

Sign up for GitHub

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

Jump to bottom

allow using a toml config file #429

Merged
merged 5 commits into from
Mar 12, 2024
Merged

allow using a toml config file #429

merged 5 commits into from
Mar 12, 2024

Conversation

orgr
Copy link
Contributor

@orgr orgr commented Mar 4, 2024

resolves #418

coderofstuff
coderofstuff requested changes Mar 7, 2024
View reviewed changes
Copy link
Collaborator

@coderofstuff coderofstuff left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. In addition to the inline comments, please also check out the TODO below.

TODO

  • Update the README.md to mention that a config file can now be used, what format it has to be, and show a sample of how to use it.

kaspad/src/args.rs
@@ -176,6 +178,7 @@ pub fn cli() -> Command {
let cmd = Command::new("kaspad")
.about(format!("{} (rusty-kaspa) v{}", env!("CARGO_PKG_DESCRIPTION"), version()))
.version(env!("CARGO_PKG_VERSION"))
.arg(arg!(-C --configfile <CONFIG_FILE> "Path of config file."))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I created a config.toml file and tried these params as they are the ones I would use for testnet-11:

testnet=true
utxoindex=true
disable-upnp=true
perf-metrics=true
unsaferpc=true
appdir = "tn11-test"
netsuffix=11
loglevel="info,kaspad_lib::daemon=trace"

appdir seems to be set, but the rest aren't getting set properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, now that I've tested it more thoroughly it appears that defaults take precedence if exist and clap's get_one() always return Some(default) so unwrap_or() didn't work like I'd expect.

I made a kinda hacky fix for it, it's possible that a refactor for the Args struct so that it's auto parsed using clap's Parser derive macro can be a prettier solution but I didn't want make so many changes.

kaspad/src/args.rs Outdated

if let Some(config_file) = m.get_one::<String>("configfile") {
let config_str = fs::read_to_string(config_file)?;
defaults = from_str(&config_str).map_err(|_| clap::Error::new(clap::error::ErrorKind::ValueValidation))?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

When there's a problem with the config file, the error is only error: invalid value for one of the arguments. Can this be updated so that the error is more descriptive of which config value needs to be fixed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the parsing of the config is now stricter and provides better errors, like:

error: failed parsing config file, reason: unknown field `blahblah`, expected one of `appdir`, `logdir`, `no-log-files`, `rpclisten`, `rpclisten-borsh`, `rpclisten-json`, `unsafe-rpc`, `wrpc-verbose`, `log-level`, `async-threads`, `connect-peers`, `add-peers`, `listen`, `user-agent-comments`, `utxoindex`, `reset-db`, `outbound-target`, `inbound-limit`, `rpc-max-clients`, `enable-unsynced-mining`, `enable-mainnet-mining`, `testnet`, `testnet-suffix`, `devnet`, `simnet`, `archival`, `sanity`, `yes`, `externalip`, `perf-metrics`, `perf-metrics-interval-sec`, `block-template-cache-lifetime`, `disable-upnp`, `disable-dns-seeding`, `disable-grpc`, `ram-scale`

@orgr orgr requested a review from coderofstuff March 7, 2024 23:17
coderofstuff
coderofstuff reviewed Mar 8, 2024
View reviewed changes
kaspad/src/args.rs Outdated
@@ -74,7 +76,7 @@ pub struct Args {
impl Default for Args {
fn default() -> Self {
Self {
appdir: Some("datadir".into()),
appdir: None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this change necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

a residual from testing the defaults issue, reverted.

coderofstuff
coderofstuff approved these changes Mar 8, 2024
View reviewed changes
Copy link
Collaborator

@coderofstuff coderofstuff left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Testing again with this new config file:

testnet=true
utxoindex=true
disable-upnp=true
perf-metrics=true
unsafe-rpc=true
appdir = "../tn11-test"
testnet-suffix=11
log-level="info,kaspad_lib::daemon=trace"

And it's now using all the params.

coderofstuff
coderofstuff reviewed Mar 8, 2024
View reviewed changes
README.md Outdated
# or
cargo run --release --bin kaspad -- -C /path/to/configfile.toml
```
The config file should be a list of \<CLI argument\> = \<value\> seperated by newlines.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor comments:

"seperated by newlines. for example:" -> "separated by newlines. For example:"

@orgr orgr requested a review from coderofstuff March 8, 2024 10:57
coderofstuff
coderofstuff approved these changes Mar 10, 2024
View reviewed changes
Copy link
Collaborator

@coderofstuff coderofstuff left a comment

Choose a reason for hiding this comment

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

LGTM

@aspect aspect merged commit 8b70061 into kaspanet:master Mar 12, 2024
6 checks passed
coderofstuff added a commit to coderofstuff/rusty-kaspa that referenced this pull request Mar 13, 2024
@coderofstuff
Set the default Args appdir to None
1f1c4f2 
With the change from kaspanet#429 the defaults are now used whenever
no value is passed to cli args.

appdir default value used to not matter, but now if it's set it makes
the datadir be inside the current directory
@coderofstuff coderofstuff mentioned this pull request Mar 13, 2024
Merged
aspect pushed a commit that referenced this pull request Mar 13, 2024
@coderofstuff
Set the default Args appdir to None (#432)
cb0451f 
With the change from #429 the defaults are now used whenever
no value is passed to cli args.

appdir default value used to not matter, but now if it's set it makes
the datadir be inside the current directory
smartgoo pushed a commit to smartgoo/rusty-kaspa that referenced this pull request Jun 18, 2024
@orgr
Allow using a toml config file (kaspanet#429)
966bc91 
* allow using a toml config file
* linting fix, serde lowercase WrpcNetAddress
* Fix handling of defaults, strict deserialize of config file + error message
* Update README
* Revert debug default, README fixes
smartgoo pushed a commit to smartgoo/rusty-kaspa that referenced this pull request Jun 18, 2024
@coderofstuff
Set the default Args appdir to None (kaspanet#432)
e1075df 
With the change from kaspanet#429 the defaults are now used whenever
no value is passed to cli args.

appdir default value used to not matter, but now if it's set it makes
the datadir be inside the current directory
D-Stacks pushed a commit to D-Stacks/rusty-kaspa that referenced this pull request Jul 12, 2024
@orgr @D-Stacks
Allow using a toml config file (kaspanet#429)
179f1b7 
* allow using a toml config file
* linting fix, serde lowercase WrpcNetAddress
* Fix handling of defaults, strict deserialize of config file + error message
* Update README
* Revert debug default, README fixes
D-Stacks pushed a commit to D-Stacks/rusty-kaspa that referenced this pull request Jul 12, 2024
@coderofstuff @D-Stacks
Set the default Args appdir to None (kaspanet#432)
1bb7edb 
With the change from kaspanet#429 the defaults are now used whenever
no value is passed to cli args.

appdir default value used to not matter, but now if it's set it makes
the datadir be inside the current directory
D-Stacks pushed a commit to D-Stacks/rusty-kaspa that referenced this pull request Jul 12, 2024
@orgr @D-Stacks
Allow using a toml config file (kaspanet#429)
d856545 
* allow using a toml config file
* linting fix, serde lowercase WrpcNetAddress
* Fix handling of defaults, strict deserialize of config file + error message
* Update README
* Revert debug default, README fixes
D-Stacks pushed a commit to D-Stacks/rusty-kaspa that referenced this pull request Jul 17, 2024
@orgr @D-Stacks
Allow using a toml config file (kaspanet#429)
986998c 
* allow using a toml config file
* linting fix, serde lowercase WrpcNetAddress
* Fix handling of defaults, strict deserialize of config file + error message
* Update README
* Revert debug default, README fixes
D-Stacks pushed a commit to D-Stacks/rusty-kaspa that referenced this pull request Jul 17, 2024
@coderofstuff @D-Stacks
Set the default Args appdir to None (kaspanet#432)
84348ac 
With the change from kaspanet#429 the defaults are now used whenever
no value is passed to cli args.

appdir default value used to not matter, but now if it's set it makes
the datadir be inside the current directory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aspect aspect aspect approved these changes

@coderofstuff coderofstuff coderofstuff approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support passing a config file to set flags
3 participants
@orgr @aspect @coderofstuff