Repository navigation
Upgrade CLI11 to 1.9.0 - develop - #8960
Conversation
This commit fixes #8947 by upgrading CLI11 to the latest version.
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
Failing build for gcc. See https://github.com/CLIUtils/CLI11 "Special instructions for GCC 8"
Also note license appears to have changed. Need legal review before merge.
|
@revl pointed out this is "BSD 3-Clause license" |
It's actually more complicated than those instructions suggest. On GCC8 it's #include <experimental/filesystem>
namespace std {namespace filesystem = std::experimental::filesystem; };Of course this isn't really a GCC thing, it's a standard library thing, so clang when using gnu's stdlib will also need those workarounds. i.e. you can't go by compiler, you need to actually test what works. But then if you're using llvm c++ lib, on version 8 & 7 you need So.. maybe best to disable that filesystem support across the board unless we need it. |
|
Hopefully we don't use filesystem. |
In fact, the text of the license hasn't changed in years. They just added a verbatim copy of the license to their single file distribution. |
As outlined on https://github.com/CLIUtils/CLI11, filesystem library is separate from the standard library on GCC 8. CLI11 uses the filesystem library only for the path type checks, which are not used by cleos, so it's safe to switch to the fallback implementation provided by CLI11.
No, we don't. Moreover, CLI11 provides a passable fallback implementation for the feature that they use from the filesystem library. |
cleos does not make use of the functionality provided by <filesystem> in CLI11, so it makes sense to disable it for all platforms to simplify the configuration.
|
Can you add a test for one of the problematic cases to one of the integration tests. |
This script tests that 'nodeos --help' and 'cleos --help' work.
|
For maintenance I think your test should be in python not sh. You can add it to an existing test or create a new one. |
heifner
left a comment
heifner
left a comment
There was a problem hiding this comment.
On hold until:
cleos wallet import --private-key
works without the workaround:
cleos wallet import --private-key ''
This commit restored backward compatibility with older version of CLI11, which allowed option arguments to be omitted. Cleos relies on this behavior to read passwords and private keys from stdin when they're not specified on the command line.
Change Description
This commit fixes #8947 by upgrading CLI11 to the latest version.
Consensus Changes
API Changes
Documentation Additions