Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Upgrade CLI11 to 1.9.0 - develop #8960

Merged
merged 8 commits into from
Apr 25, 2020
Merged

Upgrade CLI11 to 1.9.0 - develop #8960

merged 8 commits into from
Apr 25, 2020

Conversation

revl
Copy link
Contributor

@revl revl commented Apr 16, 2020

Change Description

This commit fixes #8947 by upgrading CLI11 to the latest version.

Consensus Changes

  • Consensus Changes

API Changes

  • API Changes

Documentation Additions

  • Documentation Additions

This commit fixes #8947 by upgrading CLI11 to the latest version.
Copy link
Contributor

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@heifner
Copy link
Contributor

heifner commented Apr 16, 2020

@revl pointed out this is "BSD 3-Clause license"

@spoonincode
Copy link
Contributor

Special instructions for GCC 8

It's actually more complicated than those instructions suggest.

On GCC8 it's -lstdc++fs but with GCC7 you also need something like

#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 -lc++fs instead. And on llvm's c++ lib before that you need -lc++experimental

So.. maybe best to disable that filesystem support across the board unless we need it.

@heifner
Copy link
Contributor

heifner commented Apr 16, 2020

Hopefully we don't use filesystem. CLI11_HAS_FILESYSTEM=0 will disable it.

@revl
Copy link
Contributor Author

revl commented Apr 16, 2020

@revl pointed out this is "BSD 3-Clause license"

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.
@revl
Copy link
Contributor Author

revl commented Apr 16, 2020

Hopefully we don't use filesystem. CLI11_HAS_FILESYSTEM=0 will disable it.

No, we don't. Moreover, CLI11 provides a passable fallback implementation for the feature that they use from the filesystem library.

@revl revl requested a review from heifner April 17, 2020 15:10
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.
@heifner
Copy link
Contributor

heifner commented Apr 17, 2020

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.
@heifner
Copy link
Contributor

heifner commented Apr 20, 2020

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.

Copy link
Contributor

@heifner heifner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@revl revl merged commit e83f0a1 into develop Apr 25, 2020
@revl revl deleted the upgrade-cli11-dev branch April 25, 2020 13:37
@revl revl mentioned this pull request Apr 28, 2020
3 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants