WIP: eliminate dependency on boost::program_options by eklitzke · Pull Request #12744 · bitcoin/bitcoin

eklitzke · 2018-03-21T06:50:07Z

This completely eliminates our dependency on boost::program_options, including the linking dependency:

$ ldd ./src/bitcoind | grep boost
	libboost_system.so.1.64.0 => /lib64/libboost_system.so.1.64.0 (0x00007f56e99ad000)
	libboost_filesystem.so.1.64.0 => /lib64/libboost_filesystem.so.1.64.0 (0x00007f56e9797000)
	libboost_thread.so.1.64.0 => /lib64/libboost_thread.so.1.64.0 (0x00007f56e956e000)
	libboost_chrono.so.1.64.0 => /lib64/libboost_chrono.so.1.64.0 (0x00007f56e936a000)

The catch is that this branch has two commits: my change from #12713, and then another commit that actually removes boost::program_options (see the second for the interesting part of this change). The only thing holding up that PR is disagreement about what to do about an obscure never-meant-to-be-supported edge case in the old option parser. Hopefully everyone's universal dislike of Boost is sufficient to achieve consensus on that issue.

This change split out from bitcoin#12689

practicalswift · 2018-03-21T07:12:20Z

Really nice! Let's get rid of Boost

Strong concept ACK

Empact · 2018-03-21T07:40:22Z

Another concept ACK

Some nits:

Could you run clang-format against your patch, there are some formatting inconsistencies:
git diff -U0 --no-color bitcoin/master | contrib/devtools/clang-format-diff.py -i -p1
It looks like GetNegatedArgs / IsArgNegated are exposed for testing purposes only. I would be in favor of removing those for the sake of simplicity - seems testing can be covered from the existing GetBoolArg interface.

Empact · 2018-03-21T08:05:19Z

I saw the use of GetNegatedArgs / IsNegatedArgs in your other PRs. IMO best to make each PR minimal / independent and rebase as needed.

jnewbery · 2018-03-21T16:23:01Z

One comment: config_file_iterator is able to parse sections: http://www.boost.org/doc/libs/1_66_0/doc/html/program_options/overview.html#id-1.3.31.5.10.2 . That feature isn't currently used in bitcoin but is required for #11862.

How much work would it be to enhance this PR to support INI-like config sections?

courtesy ping @ajtowns

ryanofsky

utACK 78bc52b

Would be good to add a simple unit test.

ryanofsky · 2018-03-21T17:17:25Z

src/util.cpp

-        setOptions.insert("*");
+        std::string line;
+        while (std::getline(config_file, line)) {
+            size_t eqpos = line.find('=');


Current syntax seems to be described here:

http://www.boost.org/doc/libs/1_66_0/doc/html/program_options/overview.html#id-1.3.31.5.10.2

I wonder if for backwards compatibility or convenience the new implementation should be extended to support # comments.

eklitzke · 2018-03-21T23:47:08Z

@jnewbery Seems pretty easy to add sections, INI format is literally just:

foo = 'a global option'
bar = 'another option'

[Section One]
max-file-count = 100

[Section Two]
max-file-count = 150

The option parser needs to be updated to be section-aware anyway, as right now it stuffs everything in a global namespace.

I looked at how Python checks their INI parser (configparser module) and this needs to be updated to handle comments and quoting. I will add a test case. Basically it should be able to parse this file correctly: https://github.com/python/cpython/blob/master/Lib/test/cfgparser.2

laanwj · 2018-03-22T09:05:59Z

One comment: config_file_iterator is able to parse sections:

Concept ACK - though as @jnewbery already mentions, I'd prioritize #11862 first, because adding a per-network configuration mechanism is something we've wanted for a long time.

Removing boost dependencies is what we want on the long run too, but shouldn't get in the way of providing functionality to users.

Sjors · 2018-04-03T16:44:47Z

make check is happy on macOS.

Shouldn't you remove llibboost-program-options-dev from travis.yml and control/debian/control as well?

theuni · 2018-04-04T22:04:51Z

very nice! concept ACK. Will review in detail.

theuni · 2018-04-04T22:18:06Z

src/util.cpp

-    if (strValue.empty())
-        return true;
-    return (atoi(strValue) != 0);
+    return val != "0";


Looks like this will miss " 0" (whitespace), and "-0" (negative zero), which could happen as a typo on another argument.

maflcko · 2018-04-08T20:55:31Z

Needs rebase

laanwj · 2018-04-25T07:34:49Z

Removing boost dependencies is what we want on the long run too, but shouldn't get in the way of providing functionality to users.

right - needs rebase now that per-network configuration sections have been introduced.

luke-jr · 2018-05-02T02:49:27Z

Using dependencies is strictly better than re-inventing them, but all things considered (minimal size of the code, and future modification code needed for rwconf), I guess this case looks not totally unreasonable... :x

promag · 2018-05-30T00:37:26Z

Needs rebase.

ken2812221 · 2018-06-02T14:14:41Z

Concept ACK

maflcko · 2018-06-02T14:30:52Z

Closing for now. Let me know when you start working on this again, so I can reopen. Also, this is up for grabs and can be rebased by any other contributor.

f447a0a Remove program options from build system (Chun Kuan Lee) 11588c6 Replace boost program_options (Chun Kuan Lee) Pull request description: Concept from #12744, but without parsing negated options. Tree-SHA512: 7f418744bb8934e313d77a5f162633746ef5d043de802b9c9cd9f7c1842e7e566eb5f171cd9e2cc13317281b2449c6fbd553fa4f09b837e6af2f5d2b2aabdca2

f447a0a Remove program options from build system (Chun Kuan Lee) 11588c6 Replace boost program_options (Chun Kuan Lee) Pull request description: Concept from bitcoin#12744, but without parsing negated options. Tree-SHA512: 7f418744bb8934e313d77a5f162633746ef5d043de802b9c9cd9f7c1842e7e566eb5f171cd9e2cc13317281b2449c6fbd553fa4f09b837e6af2f5d2b2aabdca2

fanquake added the Refactoring label Mar 21, 2018

eklitzke force-pushed the program_options branch 2 times, most recently from 2d68062 to 6ae9530 Compare March 21, 2018 06:58

eklitzke added 2 commits March 21, 2018 00:05

Improve boolean option parsing

6b95d6b

This change split out from bitcoin#12689

Completely eliminate boost::program_options dependency

78bc52b

eklitzke force-pushed the program_options branch from 6ae9530 to 78bc52b Compare March 21, 2018 07:09

fanquake requested review from laanwj and theuni March 21, 2018 07:16

bitcoin deleted a comment from ilike2bottom40 Mar 21, 2018

ryanofsky reviewed Mar 21, 2018

View reviewed changes

eklitzke changed the title ~~Completely eliminate dependency on boost::program_options~~ WIP: eliminate dependency on boost::program_options Mar 21, 2018

theuni reviewed Apr 4, 2018

View reviewed changes

laanwj mentioned this pull request May 2, 2018

# cannot be used rpcpassword (or bitcoin.conf in general) #13143

Closed

maflcko closed this Jun 2, 2018

ken2812221 mentioned this pull request Jun 16, 2018

Remove boost::program_options dependency #13482

Merged

HashUnlimited mentioned this pull request Sep 12, 2018

Remove boost::program_options dependency chaincoin/chaincoin#554

Merged

meshcollider added Up for grabs and removed Up for grabs labels Oct 16, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Conversation

eklitzke commented Mar 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Mar 21, 2018

Uh oh!

Empact commented Mar 21, 2018

Uh oh!

Empact commented Mar 21, 2018

Uh oh!

jnewbery commented Mar 21, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 21, 2018

Choose a reason for hiding this comment

Uh oh!

theuni Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

eklitzke commented Mar 21, 2018

Uh oh!

laanwj commented Mar 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 3, 2018

Uh oh!

theuni commented Apr 4, 2018

Uh oh!

theuni Apr 4, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Apr 8, 2018

Uh oh!

laanwj commented Apr 25, 2018

Uh oh!

luke-jr commented May 2, 2018

Uh oh!

promag commented May 30, 2018

Uh oh!

ken2812221 commented Jun 2, 2018

Uh oh!

maflcko commented Jun 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

eklitzke commented Mar 21, 2018 •

edited

Loading

laanwj commented Mar 22, 2018 •

edited

Loading