Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
New -includeconf argument for including external configuration files #10267
Conversation
|
Yeah. Why not. This can be useful.
|
|
Why not making the existing |
|
@jonasschnelli Good point - will switch to @NicolasDorier I think |
|
Unsquashed history: 1 → 2 → 3⊱1 → 4⊱2 |
|
Concept ACK
Yes, making it relative to the data directory is a good choice. I think we should handle all relative paths in
That was also my first thought, but it may just be confusing as it changes the meaning of the option slightly. It's possible that some setups already use multiple So I'm good with making it an explicit option. Another suggestion for the name would be |
|
Concept ACK. Don't care much about the name, but what about -extraconf ? |
|
From the given suggestions I think
To clarify, you mean that the relative path inside /dir/file.conf should be /dir/, not [bitcoin datadir], right? It will require some lines of code I bet but I think that makes sense too. |
|
The with |
kallewoof
changed the title from
New -readconfig argument for including external configuration files to New -includeconf argument for including external configuration files
Apr 25, 2017
Yes, seems good to me too. So it's like C's include "" - I wasn't thinking about relative includes in other includes. |
|
To clarify, the code now does what @laanwj suggested. |
laanwj
added the
RPC/REST/ZMQ
label
Apr 25, 2017
Some ideas:
To achieve the latter the option |
|
@laanwj's suggested test method seems sensible. I'm happy to review that or lend a hand implementing it. Feel free to reach me on IRC. |
|
Thanks for the suggestion! I added a test that checks for load order and ensures circular include is guarded against. @jnewbery review would be wonderful :) Edit: If anyone has ideas why travis is failing I'd appreciate it. It works fine on all the machines I test it on locally (mac, linux). |
You've made There's already a function that locks in the other order: If those two functions are called in different threads, we'd have a deadlock. There's a CPP_FLAG option that checks lock ordering You can fix this by not locking cs_args recursively. |
|
@jnewbery Thanks a lot for the explanation! I should've paid closer attention to locks considering the added recursiveness. 97ee63b fixes this by moving the conditionally-locked code into a new (Also had to tweak tests a tiny bit; 8fb6511.) |
|
I think you've introduced a subtle bug here. If I think you should try to not make ReadConfigFile recursive. For me, it would be acceptable to only allow one level of redirection here (ie the "base" config file can specify |
|
@jnewbery Hm, no the datadir cache is cleared after any recursions happen, which means it is always cleared, just not directly after the config file has been parsed. There are two cases:
As for forbidding multiple levels of recursion, I think the value outweighs the issues personally (and I addressed circular refs I believe), but if people think it's not worth it I'll restrict it to one include. |
|
@kallewoof yes you're right. datadir cache is cleared after all files are read. My mistake. I still don't like the recursion and the fact that there can be multiple levels of imports. It means there are more edge cases and unexpected behaviour. For example, if The new Finally, you've introduced a new crash bug. If If I use an invalid filename for |
|
@jnewbery Thanks a lot for all the feedback.
That is the case without recursion as well, unless we forbid command line case. (Which I can do already, even while keeping recursion.)
Hm.. the warnOnFailure was just a nice-to-have to inform the user when an explicitly included file didn't actually exist, but I can remove it for cleanliness. Since I moved most of ReadConfigFile into ReadConfigStream, the only remaining stuff was the caching stuff, which doesn't feel odd to me. I'm not actually sure why you consider this to be a problem: the ReadConfigFile is mostly there to do or not do the locking and data cache clearing, and the ReadConfigStream is there to do the actual reading/parsing part. That said, I'm not overly attached to the idea of allowing recursion, so unless someone speaks for it I am going to try to simplify the code to only allow one single include and to only allow it in the file, i.e. not from command line. I believe that would address most of your concerns. Edit: Oh, and thanks for finding the crash -- I was sure I tested that, but I guess not. Edit 2: Yeah, I never tested the case where the path was not a valid path, only when it was a non-existent one. |
jnewbery
reviewed
May 31, 2017
This looks much much better. Thanks @kallewoof! I much prefer this implementation, which restricts the way --includeconf can be used and cuts out lots of edge cases.
If it turns out that people need more flexibility, we can extend this later to allow multiple includeconfs, including multiple levels. That's much easier than going the other way (from less restrictive to more restrictive).
I have a few nits, mainly around naming.
I think there's one bug, or at least confusing behaviour. You said in your comment that you don't allow -includeconf from the command line, but your new code is allowing that. Further, if an -includeconf is included on the command line and in the base config file, then the command line -importconf takes precedence. I don't think that's what we want. To fix that, I think the first thing ReadConfigFile() should do clear the -includeconf argument if it's set.
| @@ -592,26 +592,41 @@ fs::path GetConfigFile(const std::string& confPath) | ||
| return pathConfigFile; | ||
| } | ||
| +void ArgsManager::ReadConfigStream(fs::ifstream& streamConfig) |
jnewbery
May 31, 2017
Member
I'd prefer to name this function ReadConfigFile(). At the moment, the only type of input it can read is a file, so naming it ReadInputStream() is a little misleading.
kallewoof
Jun 1, 2017
Contributor
I named if for the fact it got anifstream as argument, but you're right that it's probably better to just name it ReadConfigFile.
| @@ -592,26 +592,41 @@ fs::path GetConfigFile(const std::string& confPath) | ||
| return pathConfigFile; | ||
| } | ||
| +void ArgsManager::ReadConfigStream(fs::ifstream& streamConfig) | ||
| +{ | ||
| + std::set<std::string> setOptions; |
jnewbery
May 31, 2017
Member
Is it possible to add a log to this function to output which file it's reading, or is it too early to start logging?
Sorry - by my earlier comment, I didn't mean you should remove the logging on failure, just that the structure of the function suggested to me that it shouldn't be called recursively.
kallewoof
Jun 1, 2017
Contributor
I am logging before the call to this method now. That should cover it I think.
| + mapMultiArgs[strKey].push_back(strValue); | ||
| + } | ||
| +} | ||
| + | ||
| void ArgsManager::ReadConfigFile(const std::string& confPath) |
jnewbery
May 31, 2017
Member
I think it'd be clearer to rename this to ReadConfigFiles(). You could remove the confPath argument since it's a property of the ArgsManager class. The three places where this is called can just call gArgs.ReadConfigFiles().
The responsibilities of this function then becomes very clear: read all the config files.
| + if (mapArgs.count("-includeconf")) includeconf = mapArgs["-includeconf"]; | ||
| + } | ||
| + if (includeconf != "") { | ||
| + fs::path includeFile = GetConfigFile(includeconf); |
jnewbery
May 31, 2017
Member
Why not directly:
fs::ifstream includeConfig(GetConfigFile(includeconf));
(like above)
| + if (includeconf != "") { | ||
| + fs::path includeFile = GetConfigFile(includeconf); | ||
| + fs::ifstream includeConfig(includeFile); | ||
| + if (includeConfig.good()) { |
jnewbery
May 31, 2017
Member
Can we have an else clause that prints an error message if we fail to open an includeconfig file?
| @@ -0,0 +1,43 @@ | ||
| +#!/usr/bin/env python3 |
jnewbery
May 31, 2017
Member
It'd be nice to have a couple of other subtests:
- including
--includeconfin the command line argument has no effect - including
includeconfin an includeconf file has no effect.
kallewoof
Jun 1, 2017
•
Contributor
I'm not sure what a neat and tidy way to test that would look like. Should I just make multiple classes, one for each, and then call them one at a time in the bottom if __name__ == '__main__':?
jnewbery
Jun 1, 2017
Member
These don't actually need to be separate tests. You can just have the following in setup_chain():
with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
f.write("uacomment=relative\nincludeconf=relativeofrelative.conf\n")
with open(os.path.join(self.options.tmpdir + "/node0", "bitcoin.conf"), 'a', encoding='utf8') as f:
f.write("uacomment=main\nincludeconf=relative.conf\n")
with open(os.path.join(self.options.tmpdir + "/node0", "commandline.conf"), "w", encoding="utf8") as f:
f.write("uacomment=commandline\n")
with open(os.path.join(self.options.tmpdir + "/node0", "relativeofrelative.conf"), "w", encoding="utf8") as f:
f.write("uacomment=relativeofrelative\n")and the following line in __init__():
self.extra_args = [['-includeconf=commandline.conf']]and then keep the same assert:
assert subversion.endswith("main; relative)/")to check that neither comandline.conf nor relativeofrelative.conf have been included.
(I recommend you also update the docstring to describe what's happening)
| + super().setup_chain() | ||
| + # Create additional config file | ||
| + # - tmpdir/node0/relative.conf | ||
| + with open(os.path.join(self.options.tmpdir+"/node0", "relative.conf"), "w", encoding="utf8") as f: |
| + self.setup_clean_chain = False | ||
| + self.num_nodes = 1 | ||
| + | ||
| + def setup_network(self): |
jnewbery
May 31, 2017
Member
not required. setup_network() in the base class just calls setup_nodes() when there's only one node.
| + def setup_network(self): | ||
| + self.setup_nodes() | ||
| + | ||
| + def run_test (self): |
| + self.setup_nodes() | ||
| + | ||
| + def run_test (self): | ||
| + ''' |
jnewbery
May 31, 2017
Member
My personal preference is to have the description of the test in the module-level doc string (since that's the first thing people see when they open the file).
|
Updated and squashed. Unsquashed history. |
|
Looks great. Tested ACK bc4f7a4 One suggestion for adding to the testcase. Up to you whether you want to take it. |
|
This conflicts a little bit with 7246fae There I use the old ArgsManager::ReadConfigFile(path) which this PR remove. Or perhaps I should restore it later if this PR gets merged first. |
| + LOCK(cs_args); | ||
| + ReadConfigFile(includeConfig); | ||
| + } else { | ||
| + LogPrintf("Failed to include configuration file %s\n", includeconf.c_str()); | ||
| } | ||
| } | ||
| // If datadir is changed in .conf file: |
jtimon
Jun 1, 2017
•
Member
Shouldn't ClearDatadirCache() be called from ArgsManager::ReadConfigFile(fs::ifstream& streamConfig) ?
kallewoof
Jun 5, 2017
Contributor
ReadConfigFile is only called from ReadConfigFiles, which clears the data cache already, so it should be fine I think.
jtimon
Jun 5, 2017
Member
Yes, unless you conserve a ArgsManager::ReadConfigFile(const std::string& confPath), then that will presumably call call ArgsManager::ReadConfigFile(fs::ifstream& streamConfig too.
|
@jtimon The method was not removed, it was renamed. If you change to |
|
The method that is removed is the one that allows you to call with a path, |
jnewbery
referenced
this pull request
Jun 5, 2017
Open
Testchains: Introduce custom chain whose constructor... #8994
|
I think ReadConfigFiles() shouldn't take any arguments and should be responsible for finding and reading all config files. I don't understand why you'd want to read config files from other places in the codebase in #8994. It seems to me to be much simpler to reason about what config is loaded if it all happens in one place. |
|
@kallewoof yeah, I think that would work too, and you could still call it without parameters. That solution is very simple for me to "restore" on #8994 if people don't like it here. There's no need to slow this down if other people don't like my request. Thank you for offering a good and simple solution to my concern. @jnewbery I don't want the chain custom parameters to be perceived as "config". They select the chain you will be on, it is mostly intended to create new testnets, and sharing a "testnet config file" for a newly created one could be a useful thing. But perhaps that's something to discuss on #8994 rather than here. I still have it on a separated commit in case people prefer to allow consensus critical parameters to be passed from command line or the other config files that con be loaded. |
I actually think having the customchain config file contain general config could be useful. There seemed to be some enthusiasm for #9374 , which is similar in nature - it allows a separate config file for each separate chain. This is a bit of a sidetrack from this PR though, which I think is a good and useful improvement. This needs rebasing because of a conflict in test_runner.py. Assuming just that needs changing, then I still ACK this. I'll give some more feedback in #9374. |
|
alternative: this PR could move the: lines into the new #9374 could then call |
|
@jnewbery I would need to either change the return value to be a success flag for Rebased, btw. |
kallewoof commentedApr 24, 2017
•
edited
Fixes: #10071.
Done:
-includeconf=<path>, where<path>is relative todatadiror to the path of the file being read, if in a fileThoughts:I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.