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

[Logging] Only log "Using config file PATH_TO_bitcoin.conf" message on startup if conf file exists #14057

Conversation

Projects
None yet
7 participants
@leishman
Copy link
Contributor

commented Aug 24, 2018

Currently we log a message indicating that a bitcoin.conf file is being used even if one does not exist. This PR changes the logic to:

If config file does not exist and no -conf flag passed, log:
Config file: FILE_PATH (not found, skipping). Where FILE_PATH is the default or the path passed in with the -conf flag.

If config file does not exist and -conf flag passed with incorrect path, log warning:
Warning: The specified config file FILE_PATH does not exist

If config file exists, log:
Config file: FILE_PATH

Note: This is a (modified) subset of changes introduced in #13761 which creates a default example config file. I think it makes sense to extract this small bit out into a separate PR.

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #13761 (Docs: Create default bitcoin.conf file on startup by leishman)
  • #13746 (-masterdatadir for datadir bootstrapping by kallewoof)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 25, 2018

Concept ACK

Perhaps we should get rid of the else-case log message (“Not using config file”) to increase signal to noise in our logging?

@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

@practicalswift I'm ok with removing the else statement, but the reason I added this is to improve the user experience. Currently a user has no reliable feedback regarding where bitcoind is getting config values. If an invalid conf file path is passed in, bitcoind doesn't tell the user this file doesn't exist and says it's using the file. If we remove the else statement, the user will have no idea if they pass an incorrect path to -conf.

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

tACK ac7771d4388ca82a4e36fc058cb328b2860ed7c9 on macOS (testing Windows might a good idea).

@practicalswift maybe we could hide the “Not using config file” log message if -conf is not explicitly set (!gArgs.IsArgSet("-conf"))?

A future update could abort if -conf is explicitly set by the user to a non-existent path, but for now at least the log message is more useful.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 26, 2018

@leishman Good point! I think @Sjors suggestion is a good compromise – to show the “Not using config file” only if -conf is set.

utACK ac7771d4388ca82a4e36fc058cb328b2860ed7c9 assuming @Sjors suggestion is implemented :-)

@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 26, 2018

@practicalswift @Sjors the reason i didn't add that extra condition was for the following scenario: A user creates a bitcoin.conf file in the wrong directory and they can't figure out why bitcoind is not using their settings. At least with this change they will be able to see where bitcoind is looking by default for a configuration file and they will know that bitcoind does not see what it's looking for.

@Sjors

This comment has been minimized.

Copy link
Member

commented Aug 28, 2018

@leishman that makes sense. Perhaps if -conf is not set, the error could say "No file exists at the default location %s", but maybe that's overkill.

@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 29, 2018

@Sjors I don't think it hurts, as this is a known source of confusion for new users. I've updated with your suggestion and tested on Mac. Anyone have a windows machine they can test on?

Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated
Show resolved Hide resolved src/init.cpp Outdated
@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

@practicalswift thanks for the suggestions and for catching the typo. I've updated accordingly.

Show resolved Hide resolved src/init.cpp Outdated
Only log "Using PATH_TO_bitcoin.conf" message on startup if conf file…
… exists.

Currently we log a message indicating that a bitcoin.conf file is being used
even if one does not exists. This commit changes the logic to only display
this message if a config file exists and logs a separate message
if no config file exists. Additionally, a warning is now logged if the file
path passed in the -conf flag does not exist.
@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

utACK 8b5a693db76b70baf5964df3609b5218cc3711a8

@leishman leishman force-pushed the leishman:leishman--only-show-conf-message-if-file-exists branch to 946107a Aug 30, 2018

@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

@practicalswift @Sjors rebased to clean up commit history

@practicalswift

This comment has been minimized.

Copy link
Member

commented Aug 30, 2018

utACK 946107a

@Sjors

This comment has been minimized.

Copy link
Member

commented Sep 2, 2018

tACK 946107a

Note that InitWarning causes QT to pop up a warning if you give it an invalid config path (which makes sense):
schermafbeelding 2018-09-02 om 09 53 57

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Sep 5, 2018

utACK 946107a

@leishman

This comment has been minimized.

Copy link
Contributor Author

commented Sep 9, 2018

@laanwj is this good to merge or is there anything else you'd like to see tested?

@laanwj

This comment has been minimized.

Copy link
Member

commented Sep 10, 2018

utACK 946107a

@laanwj laanwj merged commit 946107a into bitcoin:master Sep 10, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

ken2812221 pushed a commit to ken2812221/bitcoin that referenced this pull request Sep 10, 2018

Merge bitcoin#14057: [Logging] Only log "Using config file PATH_TO_bi…
…tcoin.conf" message on startup if conf file exists

946107a Only log "Using PATH_TO_bitcoin.conf" message on startup if conf file exists. (Alexander Leishman)

Pull request description:

  Currently we log a message indicating that a bitcoin.conf file is being used even if one does not exist. This PR changes the logic to:

  **If config file does not exist and no -conf flag passed, log:**
  `Config file: FILE_PATH (not found, skipping)`. Where `FILE_PATH` is the default or the path passed in with the `-conf` flag.

  **If config file does not exist and -conf flag passed with incorrect path, log warning:**
  `Warning: The specified config file FILE_PATH does not exist`

  **If config file exists, log**:
  `Config file: FILE_PATH`

  Note: This is a (modified) subset of changes introduced in bitcoin#13761 which creates a default example config file. I think it makes sense to extract this small bit out into a separate PR.

Tree-SHA512: be0f0ae6a0c9041e2d6acb54d2563bbcc79786fb2f8bf9a963fe01bc54cd4e388b89079fde1eb79f7f17099776428e5e984bf7107590a3d1ecfc0562dbc6e3f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.