Store the current config file at read time, and return it from GetConfigFile if set #12765

Empact commented at 12:14 PM on March 23, 2018: member

This maintains accuracy in the context of datadirectory changes, e.g. #12722

Also drop the related arguments to GetConfigFile and ReadConfigFile, and follow the example of GetDebugLogPath by calling GetArg within the GetConfigFile function.

fanquake added the label Utils/log/libs on Mar 23, 2018

ryanofsky commented at 3:18 PM on March 23, 2018: member

utACK f73aed2d78391bcf8aeaa16e69f72d74e9dfb163. This is a nice fix for #12722 and also seems like a good code simplification.

Empact force-pushed on Mar 24, 2018

Empact commented at 5:57 AM on March 24, 2018: member

Added some testing for ReadConfigFile, which wasn't previously covered.

Empact force-pushed on Mar 24, 2018

MarcoFalke commented at 12:26 PM on March 24, 2018: member

Haven't looked closely, but this seems to revert #8856

in src/test/getarg_tests.cpp:39 in ca647962b7 outdated

  36 | +  args << "-conf=" << (fs::path("data") / "bitcoin.conf").string()
  37 | +       << " -datadir=" << fs::path("src/test").string();
  38 | +  ResetArgs(args.str());
  39 | +  gArgs.ReadConfigFile();
  40 | +
  41 | +  BOOST_CHECK_EQUAL(gArgs.GetConfigFile(), fs::system_complete("test/data/bitcoin.conf"));

MarcoFalke commented at 12:28 PM on March 24, 2018:

test/getarg_tests.cpp(41): error: in 
"getarg_tests/configfile": check gArgs.GetConfigFile() == fs::system_complete("test/data/bitcoin.conf") has failed
["/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/src/data/bitcoin.conf" != "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/src/test/data/bitcoin.conf"]

Empact commented at 9:57 PM on March 24, 2018:

Yeah it's running different locally for me than on CI, and have not figured that out yet. In cases where the paths match up as expected (which should mean the config is properly identified), the arg checks do not pass, as though the file is not being picked up.

Empact commented at 12:49 PM on March 25, 2018:

Any idea what's going on here? https://travis-ci.org/bitcoin/bitcoin/jobs/358040567

It's saying the test bitcoin.conf does not exist under CI. (Note the "test/data/bitcoin.conf" comparator is just thrown in to cause a failure)

test/getarg_tests.cpp(39): error in "configfile": check fs::exists(gArgs.GetConfigFile()) failed
test/getarg_tests.cpp(40): error in "configfile": check gArgs.GetConfigFile() == "test/data/bitcoin.conf" failed ["/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/src/test/data/bitcoin.conf" != test/data/bitcoin.conf]

Empact force-pushed on Mar 24, 2018

Empact force-pushed on Mar 25, 2018

Empact force-pushed on Mar 26, 2018

Empact commented at 1:57 AM on March 27, 2018: member

Re #8856, seems the file has gone the other direction since then - maybe due to the introduction of ArgsManager.

Currently the following other util methods use GetArg:

GetDataDir
GetPidFile
GetDebugLogPath

In this case seems the change is required if we want to retain the state.

Empact force-pushed on Mar 27, 2018

in src/util.cpp:698 in 4692fc0100 outdated

 694 | @@ -690,6 +695,7 @@ void ArgsManager::ReadConfigFile()
 695 |      if (!fs::is_directory(GetDataDir(false))) {
 696 |          throw std::runtime_error(strprintf("specified data directory \"%s\" does not exist.", gArgs.GetArg("-datadir", "").c_str()));
 697 |      }
 698 | +    return;

ryanofsky commented at 8:54 PM on March 28, 2018:

Store the current config file at read time

Unneeded return

ryanofsky commented at 9:10 PM on March 28, 2018: member

utACK 4692fc01003e39719c50632ba16339fd5173a856. Only difference since last review is adding tests (thanks!), declaring GetConfigFile const, and some formatting changes.

#8856 doesn't match current organization of the code not so relevant. Following that approach seems more repetitive, and also like it would prevent fixing the bug here.

Empact force-pushed on Mar 28, 2018

Empact force-pushed on Mar 30, 2018

Empact force-pushed on Mar 31, 2018

ryanofsky commented at 3:31 PM on April 3, 2018: member

utACK 900e3e54890ed3d2c9ba3362794c928bfdc90151. Just rebase, formatting changes, and new config file test since last review.

This PR depends on #12812 which still has some comments that need to be addressed there.

Empact commented at 1:38 AM on April 5, 2018: member

Thinking to hold this pending #12878

MarcoFalke commented at 12:03 PM on April 9, 2018: member

Needs rebase if still relevant

Empact force-pushed on Apr 11, 2018

Empact commented at 2:00 AM on April 11, 2018: member

Rebased and updated tests to package the config file via the TEST_FILES_* setup.

Empact force-pushed on Apr 11, 2018

ajtowns commented at 2:16 PM on April 11, 2018: member

Two thoughts:

rather than have GetConfigFile use gArgs, probably better to make GetConfigFile a member of ArgsManager?
might make sense to use to split between config args and command line args in #11862 and have GetConfigFile/ReadConfigFile only lookup command line args for the value for "-conf", rather than saving it separately?

MarcoFalke commented at 7:31 PM on April 16, 2018: member

Sorry, needs rebase again.

Empact force-pushed on May 3, 2018

Empact force-pushed on May 21, 2018

MarcoFalke commented at 2:00 PM on May 30, 2018: member

Needs rebase due to merge of #13341

Store the current config file at read time

And return it from GetConfigFile if set.

This maintains accuracy in the context of datadirectory changes.

Note this drops the argument GetConfigFile, which is consistent
with other ArgsManager methods which read their own configuration
variable, e.g. GetDebugLogPath.

63594c32e9

Empact force-pushed on May 31, 2018

DrahtBot added the label Needs rebase on Jul 20, 2018

DrahtBot commented at 10:39 PM on July 20, 2018: member

Needs rebase

Empact closed this on Oct 28, 2018

MarcoFalke added the label Up for grabs on Oct 28, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

DrahtBot locked this on Dec 16, 2021