[docs] [refactor] Add help messages for datadir path mangling #12305

jamesob · 2018-01-30T22:09:00Z

Change -conf's help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with -conf=bitcoin.conf, but instead loading the bitcoin.conf file in ~/.bitcoin datadir.

Edit

This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization.

laanwj · 2018-01-30T22:13:18Z

Not just this one, all relative files and directories specified either on the command line or through RPC are supposed to be relative to the data directory.

maflcko · 2018-01-30T22:27:21Z

OT: Wasn't walletdir temporarily allowed to be relative to the cwd?

jamesob · 2018-01-30T22:38:10Z

Sounds like we don't have any documentation indicating that all path arguments are relative to datadir; that information should live somewhere (preferably in a single place that isn't easy to miss). One option is to put it in a paragraph between "Usage" and "Options" in {bitcoind,bitcoin-cli,bitcoin-qt} -h. Any other suggestions?

maflcko · 2018-01-30T23:22:31Z

Options that take paths or file names are:

-rpccookiefile=<loc>
-walletdir=<dir>
-wallet=<file>
-datadir=<dir>
-conf=<file>

So, adding a general paragraph only makes sense if it holds true for all paths. Otherwise, I'd prefer if each command line help was clarified individually.

jamesob · 2018-01-31T04:02:37Z

@MarcoFalke thanks for that list. I've looked at how each option on the list is parsed (as well as a few others):

-rpccookiefile=<loc>: Is put under datadir if relative.
-walletdir=<dir>: Isn't allowed to be relative.
-wallet=<file>: Moved under walletdir with fs::absolute(walletFile, GetWalletDir()).
-datadir=<dir>: Can be specified as relative but will warn. Obviously not put under itself.
-conf=<file>: Is put under datadir if relative.
-loadblock=<file>: Interpreted literally -- isn't modified if relative. Passed directly to fsbridge::fopen.
-debuglogfile=<file>: Is put under datadir if relative.
-pid=<file>: Is put under datadir if relative.

So: each of these options is either not allowed to be relative or is put under datadir if relative with the exception of wallet and loadblock. Because we don't uniformly treat relative paths as relative to datadir, I've updated only the affected help messages.

I've also added a refactoring commit which consolidates the "normalization" of relative paths under datadir.

laanwj · 2018-01-31T09:47:29Z

Thanks, utACK

promag

Concept ACK.

promag · 2018-01-31T15:48:12Z

src/util.cpp

I'd rename to GetAbsolutePath or more specific GetAbsoluteDataPath:

fs::path GetAbsoluteDataPath(const fs::path& path, bool net_specific)

I other contexts/languages, normalization can be interpreted as transforming from foo/../bar/ to bar.

Nit, snake_case arguments.

promag · 2018-01-31T15:48:41Z

src/wallet/init.cpp

Nit, unrelated change, remove.

jamesob · 2018-01-31T16:15:21Z

Thanks for the feedback @promag. It's been incorporated and pushed.

ryanofsky

utACK 23d295aa9e16cb3591bbd8bb0f5ce026b861b7b6

ryanofsky · 2018-01-31T18:08:44Z

src/util.cpp

Would be a little better to return: return fs::absolute(path, GetDataDir(net_specific)) to make it clear this returns an absolute path. You could also drop the is_complete check above if you did this.

Great, thanks.

ryanofsky · 2018-01-31T18:10:55Z

src/util.cpp

Could eliminate this check, but if you will keep it developer notes suggest adding braces or putting if body on same line as condition.

Taking your advice below, but the reminder about convention is appreciated.

ryanofsky · 2018-01-31T18:12:28Z

src/util.h

"ForArg" in function name seems a little redundant. Just a suggestion, but I might call it something GetDataPath or GetDataDirPath.

My only reservation here is that the path won't necessarily be in datadir, so a name with Data or DataDir seems confusing. "Arg" here is meant to correspond to an argument to the binary, not the function, but maybe that isn't clear.

I'll try AbsPathForConfigVal.

jamesob · 2018-01-31T20:41:28Z

Thanks for the good feedback, @ryanofsky. Incorporated and pushed.

ajtowns · 2018-02-02T10:14:11Z

Could update the commit title to match the renamed function :)

jamesob · 2018-02-02T14:24:46Z

d'oh. Thanks, @ajtowns. Pushed.

jnewbery · 2018-02-05T20:07:04Z

Looks good.

Minor note that -conf is relative to the base datadir, but -debuglogfile, -pid and -rpccookiefile are relative the net-specific subdirectories. It might be better to specify that in the help text.

sipa · 2018-02-05T21:10:07Z

utACK 95963c120c48ce750fdb2298f3eb70034e578e8d, but needs rebase.

Change `-conf`'s and others' help messages to indicate that relative path values will be prefixed by the datadir path. This behavior is confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but loading the `bitcoin.conf` file in ~/.bitcoin datadir.

Most commandline/config args are interpreted as relative to datadir if not passed absolute. Consolidate the logic for this normalization.

jamesob · 2018-02-05T22:50:27Z

Rebased and incorporated @jnewbery's feedback.

promag · 2018-02-05T23:13:04Z

src/util.h

+ *
+ * @param path The path to be conditionally prefixed with datadir.
+ * @param net_specific Forwarded to GetDataDir().
+ * @return The normalized path.


The absolute path?

jnewbery · 2018-02-06T00:36:47Z

utACK 5460460

…ngling 5460460 Add AbsPathForConfigVal to consolidate datadir prefixing for path args (James O'Beirne) a1e1305 Clarify help messages for path args to mention datadir prefix (James O'Beirne) Pull request description: Change `-conf`'s help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but instead loading the `bitcoin.conf` file in ~/.bitcoin datadir. ### Edit This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization. Tree-SHA512: be4fc0595fbeba33d17af08f59898af45e76a44f00719ea0282403b155ac6755584604fab765250a3aa14ed6991882c4d1ccbe601184362c5ba97c886bdda344

maflcko · 2018-02-06T23:05:28Z

Post-merge utACK 5460460

…ngling Summary: 5460460 Add AbsPathForConfigVal to consolidate datadir prefixing for path args (James O'Beirne) a1e1305 Clarify help messages for path args to mention datadir prefix (James O'Beirne) Pull request description: Change `-conf`'s help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but instead loading the `bitcoin.conf` file in ~/.bitcoin datadir. ### Edit This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization. Tree-SHA512: be4fc0595fbeba33d17af08f59898af45e76a44f00719ea0282403b155ac6755584604fab765250a3aa14ed6991882c4d1ccbe601184362c5ba97c886bdda344 Backport of Core PR12305 bitcoin/bitcoin#12305 Depends on D3940 Test Plan: make check ./bitcoind -h Verify help messages have been changed. ./bitcoind Verify debug.log, bitcoin.conf, bitcoind.pid, and cookie files exist in their default directories (Likely somewhere in .bitcoin). ./bitcoind -debuglogfile=<absolute path>/debug.log Verify the same debug.log was touched by this instance of `./bitcoind`. ./bitcoind -debuglogfile=<relative path>/debug.log Verify the same debug.log was touched by this instance of `./bitcoind`. Repeat for other files. Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3939

…path mangling 5460460 Add AbsPathForConfigVal to consolidate datadir prefixing for path args (James O'Beirne) a1e1305 Clarify help messages for path args to mention datadir prefix (James O'Beirne) Pull request description: Change `-conf`'s help message to indicate that relative path values will be prefixed by the datadir path. This behavior probably merits clarification; it's kind of confusing when attempting to specify a configuration file in the current directory with `-conf=bitcoin.conf`, but instead loading the `bitcoin.conf` file in ~/.bitcoin datadir. ### Edit This PR has been modified to document all cases where relative path configurations are modified to be under datadir. A small refactoring has also been added which consolidates this normalization. Tree-SHA512: be4fc0595fbeba33d17af08f59898af45e76a44f00719ea0282403b155ac6755584604fab765250a3aa14ed6991882c4d1ccbe601184362c5ba97c886bdda344

fanquake added the Docs label Jan 31, 2018

jamesob force-pushed the jamesob/conf-flag-path-help branch from e250904 to 8e1cf5b Compare January 31, 2018 03:54

jamesob force-pushed the jamesob/conf-flag-path-help branch from 754baf5 to d04ffdb Compare January 31, 2018 04:11

jamesob changed the title ~~Clarify -conf help message to mention datadir path prefix~~ [docs] [refactor] Add help messages for datadir path mangling Jan 31, 2018

jamesob force-pushed the jamesob/conf-flag-path-help branch from d04ffdb to d76f43c Compare January 31, 2018 04:21

promag reviewed Jan 31, 2018

View reviewed changes

jamesob force-pushed the jamesob/conf-flag-path-help branch from d76f43c to 23d295a Compare January 31, 2018 16:14

ryanofsky reviewed Jan 31, 2018

View reviewed changes

jamesob force-pushed the jamesob/conf-flag-path-help branch from 23d295a to fdf0c7d Compare January 31, 2018 20:40

jamesob force-pushed the jamesob/conf-flag-path-help branch from fdf0c7d to 95963c1 Compare February 2, 2018 14:24

jamesob added 2 commits February 5, 2018 17:48

Add AbsPathForConfigVal to consolidate datadir prefixing for path args

5460460

Most commandline/config args are interpreted as relative to datadir if not passed absolute. Consolidate the logic for this normalization.

jamesob force-pushed the jamesob/conf-flag-path-help branch from 95963c1 to 5460460 Compare February 5, 2018 22:49

promag reviewed Feb 5, 2018

View reviewed changes

laanwj merged commit 5460460 into bitcoin:master Feb 6, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

[docs] [refactor] Add help messages for datadir path mangling #12305

[docs] [refactor] Add help messages for datadir path mangling #12305

Uh oh!

Conversation

jamesob commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Edit

Uh oh!

laanwj commented Jan 30, 2018

Uh oh!

maflcko commented Jan 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob commented Jan 30, 2018

Uh oh!

maflcko commented Jan 30, 2018

Uh oh!

jamesob commented Jan 31, 2018

Uh oh!

laanwj commented Jan 31, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jan 31, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamesob commented Jan 31, 2018

Uh oh!

ajtowns commented Feb 2, 2018

Uh oh!

jamesob commented Feb 2, 2018

Uh oh!

jnewbery commented Feb 5, 2018

Uh oh!

sipa commented Feb 5, 2018

Uh oh!

jamesob commented Feb 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 6, 2018

Uh oh!

maflcko commented Feb 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

jamesob commented Jan 30, 2018 •

edited

Loading

maflcko commented Jan 30, 2018 •

edited

Loading