Add check for absolute path of config file #1276

KipHamiltons · 2020-05-07T04:23:14Z

#1071
Added absolute path capability for specifying config file.
This is my first time addressing an issue with a GitHub commit - if I have screwed up anything, please let me know! :)

ddobbelaere · 2020-05-07T08:28:02Z

src/utils/configfile.cc

@@ -88,7 +88,9 @@ bool ConfigFile::Init(OptionsParser* options) {
  // loading the default configuration file.
  if (filename == "") return true;

-  filename = CommandLine::BinaryDirectory() + "/" + filename;
+  if (filename.at(0) != '/') {


This fix will work on Unix-like operating systems, but seems to leave Windows (where absolute paths tend to start with C:\, D:\, etc) users in the cold.

I have added a preprocessor directive to check for windows, then check for a colon in that path. I don't have a way to whether the windows section is robust though.
Thanks for pointing that out though! I forgot that this project is for windows too!

ddobbelaere · 2020-05-08T06:14:29Z

Now that we support C++17, I think https://stackoverflow.com/a/43278300 is more robust and prettier (probably there exist corner cases where the second character of an absolute path on Windows is not a colon). Hmm, there is even a function std::filesystem::absolute to convert any path to its absolute form, see https://en.cppreference.com/w/cpp/filesystem/absolute, but actually we cannot use it as we have to reference w.r.t. the binary directory and not the working directory.

On a side note: the current code throws an exception which kills the engine on Windows for single character filenames.

The compiler can't find the <filesystem>. I have no idea why not.

ddobbelaere · 2020-05-08T08:10:50Z

CI on Windows systems passes. Only Android on Appveyor and Linux on Circle CI fail, probably because of this (see https://en.cppreference.com/w/cpp/filesystem):

Notes

Using this library may require additional compiler/linker options. GNU implementation prior to 9.1 requires linking with -lstdc++fs and LLVM implementation prior to LLVM 9.0 requires linking with -lc++fs

Probably @borg323 and/or @lealgo have ideas to fix this?

borg323 · 2020-05-08T08:33:16Z

Just a quick hint for now, this should probably be handled in src/utils/filesystem.posix.cc and src/utils/filesystem.win32.cc.

KipHamiltons · 2020-05-08T08:47:01Z

I initially pushed that commit because I had no idea why my compiler couldn't find <filesystem>, then I realised that I was using the wrong compiler just after I pushed.

Now that I can actually test my code, I am getting a segfault whenever I call the std::filesystem::path constructor.

for example, just having the line std::filesystem::path test = ""; in there, without using the variable creates a segfault a bit later, when the ParseFile is returning true (I didn't add a --config option).

Just a quick hint for now, this should probably be handled in src/utils/filesystem.posix.cc and src/utils/filesystem.win32.cc.

I'll have a go at porting it over (I said 'now' before, but I realise now that this isn't a copy-paste job to move it over)

ddobbelaere · 2020-05-08T10:36:58Z

Seems std::filesystem causes some headaches and/or is poorly supported. I wasn't aware of the OS specific files @borg323 pointed to. This seems indeed the cleanest and most consistent approach to do things there (e.g. make an "IsRelativePath" (or whatever) function with OS specific implementation, for Windows you can make use of its specific API calls).

mooskagh · 2020-05-08T20:34:22Z

Uh I was going to suggest https://en.cppreference.com/w/cpp/filesystem/path/is_absrel but see it's already being discussed. :)

mooskagh · 2020-05-08T20:39:26Z

Note that for Windows, both / and \ is allowed, and it's also allowed to omit D: part.
So technically what's absolute path in Linux is a subset of what's absolute path in Windows.

Maybe regex-based approach would work? :) Something like ^(?:\w:)?[\\/]. Although at least as of 3 years ago, support of regexes in C++ were roughly as problematic as filesystem part of STL seems to be now.

And technically speaking C:\windows/system32 is actually a valid relative path in Linux.. :)

mooskagh · 2020-05-08T21:16:11Z

How about making it similar to weights file autodiscover? Basically if the value is <default>, search for Lc0.config in the directory of binary. Otherwise just use the path (relative to cwd if it's relative).

KipHamiltons · 2020-05-08T23:39:35Z

I have used the check for first character / in the posix filesystem, and let the windows API decide about the windows path, as per https://stackoverflow.com/a/5377662/11108223 - although I am not sure if I need to #include <shlwapi.h> on the windows file.

How about making it similar to weights file autodiscover? Basically if the value is , search for Lc0.config in the directory of binary. Otherwise just use the path (relative to cwd if it's relative).

I have retained this functionality I think, by not messing with how it works currently for the most part.

I really appreciate all of your help everyone (not that this is necessarily done). Please keep the feedback coming - I am really enjoying my first open source experience :)

lealgo · 2020-05-09T04:27:34Z

IMO the main issue here is that the current functionality (in the release) is confusing: the config file can only reside in a path that is relative to the directory where the binary is located. This is a very weird convention. Normally paths are either absolute or relative to the current working dir. So I like @mooskagh's idea.

KipHamiltons · 2020-05-09T04:45:02Z

Ahh I think I understand how that is different to what I have done/what happens at the moment. Do you want me to go full steam ahead on that @lealgo ?

EDIT: Also, I have no idea how to show the linker where to find the windows API method which I used to determine if the path is relative. Directions would be very appreciated, as I have very little experience with building projects (university doesn't seem to think its so important lol...)

mooskagh · 2020-05-09T06:40:08Z

Yeah (unless someone objects) I think having it only search in the directory of the binary if it's "<default>" is good enough to be implemented. :)

That way we'll also be able to search multiple locations by default in future, which is useful for linux (i.e. lc0.config in the directory of the binary, then '~/.config/lc0/lc0.config, then /etc/lc0.config, then ./lc0.config`).

ddobbelaere · 2020-05-10T07:16:15Z

The current solution comes forward to what @mooskagh suggested, allows users to specify an absolute path and treats relative paths (except the default config filename itself, but this can be overruled to use the file in the working dir by using ./lc0.config) w.r.t. the working directory, which is more logical. LGTM.

Nit: there are still some stale unneeded #include statements lingering.

KipHamiltons · 2020-05-11T00:03:54Z

Nit: there are still some stale unneeded #include statements lingering.

Well spotted! cheers!

mooskagh · 2020-05-11T08:05:58Z

src/utils/configfile.cc

-  filename = CommandLine::BinaryDirectory() + "/" + filename;
+  // Check to see if we are using the default config file or not.
+  const bool using_default_config = 
+      filename == std::string(kDefaultConfigFile);


I think it's better to have a separate value for default behaviour, like "<default>" or "<auto>" (which still will read $LC0_DIRECTORY/lc0.config), similarly to what we have for weights file (there we have "<autodiscover>").

Otherwise when run like ./lc0 -c lc0.cfg it will search in the current directory, while for ./lc0 -c lc0.config it will search in the directory of the binary, which is a confusing inconsistency.

Also in future we'll try several locations by default (possibly with different file names, not only directories, e.g. /etc/lc0.cfg vs ~/.lc0rc), and it's more logical if the parameter doesn't have file name in it.

I have made changes along these lines. Let me know if this is what you were thinking 🙂

mooskagh · 2020-05-23T08:16:33Z

src/utils/configfile.cc

@@ -42,7 +42,7 @@ const OptionId kConfigFileId{
    "Path to a configuration file. The format of the file is one command line "
    "parameter per line, e.g.:\n--weights=/path/to/weights",
    'c'};
-const char* kDefaultConfigFile = "lc0.config";
+const char* kDefaultConfigFile = "<default>";


Sorry for nitpicking, but I suggest moving both constants here:

const char* kDefaultConfigFile = "lc0.config"; const char* kDefaultConfigFileParam = "<default>";

(or if it's one, better it be the filename and not the option value)

Not a problem. Do these changes match what you were thinking?

Looks good!

Add check for absolute path of config file

5109e94

ddobbelaere reviewed May 7, 2020

View reviewed changes

Added check for windows systems.

6743db9

Changed to using C++17's filepath checker - DOES NOT BUILD

104408e

The compiler can't find the <filesystem>. I have no idea why not.

KipHamiltons marked this pull request as draft May 8, 2020 07:50

Moved the file conversion to utils/filesystem

0f01c91

youngest-person-alive added 2 commits May 9, 2020 13:46

Added includ for windows API file

bbc9a02

fixed windows API call hopefully

b0360c7

Typecast string to LPCSTR for win API call

b14a0c0

youngest-person-alive added 2 commits May 10, 2020 06:35

Removed previous solution

062fbab

Use the path given, but default to lc0.config in binary directory.

0c54367

KipHamiltons marked this pull request as ready for review May 9, 2020 23:58

youngest-person-alive added 2 commits May 11, 2020 09:59

Cleaned up includes

95b081b

Removed extra line

65c23b2

mooskagh reviewed May 11, 2020

View reviewed changes

Changed default file parameter

ea7e4b8

youngest-person-alive added 2 commits May 16, 2020 18:12

Changed filename const-ness

d9adf7e

Fixed default filename

a603ec1

mooskagh approved these changes May 23, 2020

View reviewed changes

Added const default filename.

8fc4bba

mooskagh approved these changes May 28, 2020

View reviewed changes

mooskagh merged commit a3b38f9 into LeelaChessZero:master May 28, 2020

ddobbelaere mentioned this pull request Jun 17, 2020

Use filesystem of C++17 #1345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for absolute path of config file #1276

Add check for absolute path of config file #1276

KipHamiltons commented May 7, 2020 •

edited

Loading

ddobbelaere May 7, 2020 •

edited

Loading

KipHamiltons May 7, 2020 •

edited

Loading

ddobbelaere commented May 8, 2020 •

edited

Loading

ddobbelaere commented May 8, 2020

borg323 commented May 8, 2020

KipHamiltons commented May 8, 2020 •

edited

Loading

ddobbelaere commented May 8, 2020 •

edited

Loading

mooskagh commented May 8, 2020

mooskagh commented May 8, 2020

mooskagh commented May 8, 2020

KipHamiltons commented May 8, 2020

lealgo commented May 9, 2020

KipHamiltons commented May 9, 2020 •

edited

Loading

mooskagh commented May 9, 2020

ddobbelaere commented May 10, 2020 •

edited

Loading

KipHamiltons commented May 11, 2020

mooskagh May 11, 2020

KipHamiltons May 16, 2020

mooskagh May 23, 2020

KipHamiltons May 26, 2020

mooskagh May 28, 2020

Add check for absolute path of config file #1276

Add check for absolute path of config file #1276

Conversation

KipHamiltons commented May 7, 2020 • edited Loading

ddobbelaere May 7, 2020 • edited Loading

Choose a reason for hiding this comment

KipHamiltons May 7, 2020 • edited Loading

Choose a reason for hiding this comment

ddobbelaere commented May 8, 2020 • edited Loading

ddobbelaere commented May 8, 2020

borg323 commented May 8, 2020

KipHamiltons commented May 8, 2020 • edited Loading

ddobbelaere commented May 8, 2020 • edited Loading

mooskagh commented May 8, 2020

mooskagh commented May 8, 2020

mooskagh commented May 8, 2020

KipHamiltons commented May 8, 2020

lealgo commented May 9, 2020

KipHamiltons commented May 9, 2020 • edited Loading

mooskagh commented May 9, 2020

ddobbelaere commented May 10, 2020 • edited Loading

KipHamiltons commented May 11, 2020

mooskagh May 11, 2020

Choose a reason for hiding this comment

KipHamiltons May 16, 2020

Choose a reason for hiding this comment

mooskagh May 23, 2020

Choose a reason for hiding this comment

KipHamiltons May 26, 2020

Choose a reason for hiding this comment

mooskagh May 28, 2020

Choose a reason for hiding this comment

KipHamiltons commented May 7, 2020 •

edited

Loading

ddobbelaere May 7, 2020 •

edited

Loading

KipHamiltons May 7, 2020 •

edited

Loading

ddobbelaere commented May 8, 2020 •

edited

Loading

KipHamiltons commented May 8, 2020 •

edited

Loading

ddobbelaere commented May 8, 2020 •

edited

Loading

KipHamiltons commented May 9, 2020 •

edited

Loading

ddobbelaere commented May 10, 2020 •

edited

Loading