WIP: eliminate dependency on boost::program_options #12744

pull eklitzke wants to merge 2 commits into bitcoin:master from eklitzke:program_options changing 8 files +152 −170
  1. eklitzke commented at 6:50 am on March 21, 2018: contributor

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

    0$ ldd ./src/bitcoind | grep boost
1	libboost_system.so.1.64.0 => /lib64/libboost_system.so.1.64.0 (0x00007f56e99ad000)
2	libboost_filesystem.so.1.64.0 => /lib64/libboost_filesystem.so.1.64.0 (0x00007f56e9797000)
3	libboost_thread.so.1.64.0 => /lib64/libboost_thread.so.1.64.0 (0x00007f56e956e000)
4	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.

  2. fanquake added the label Refactoring on Mar 21, 2018
  3. fanquake added this to the "In progress" column in a project
  4. eklitzke force-pushed on Mar 21, 2018
  5. eklitzke force-pushed on Mar 21, 2018
  6. Improve boolean option parsing 
    This change split out from #12689
    6b95d6bd6e
  7. Completely eliminate boost::program_options dependency 78bc52bf86
  8. eklitzke force-pushed on Mar 21, 2018
  9. practicalswift commented at 7:12 am on March 21, 2018: contributor

    Really nice! Let’s get rid of Boost

    Strong concept ACK

  10. fanquake requested review from laanwj on Mar 21, 2018
  11. fanquake requested review from theuni on Mar 21, 2018
  12. Empact commented at 7:40 am on March 21, 2018: member

    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.
  13. Empact commented at 8:05 am on March 21, 2018: member
    I saw the use of GetNegatedArgs / IsNegatedArgs in your other PRs. IMO best to make each PR minimal / independent and rebase as needed.
  14. MarcoFalke deleted a comment on Mar 21, 2018
  15. jnewbery commented at 4:23 pm on March 21, 2018: member

    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

  16. in src/util.cpp:680 in 78bc52bf86 
    679         LOCK(cs_args);
680-        std::set<std::string> setOptions;
681-        setOptions.insert("*");
682+        std::string line;
683+        while (std::getline(config_file, line)) {
684+            size_t eqpos = line.find('=');
    ryanofsky commented at 5:17 pm on March 21, 2018:

    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.

    theuni commented at 10:34 pm on April 4, 2018:
    +1
  17. ryanofsky commented at 5:20 pm on March 21, 2018: member

    utACK 78bc52bf863fd0f212a9a29adb1c99651cc4a0eb

    Would be good to add a simple unit test.

  18. eklitzke commented at 11:47 pm on March 21, 2018: contributor

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

    0foo = 'a global option'
1bar = 'another option'
2
3[Section One]
4max-file-count = 100
5
6[Section Two]
7max-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

  19. eklitzke renamed this:
    Completely eliminate dependency on boost::program_options
    WIP: eliminate dependency on boost::program_options
    on Mar 21, 2018
  20. laanwj commented at 9:05 am on March 22, 2018: member

    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.

  21. Sjors commented at 4:44 pm on April 3, 2018: member

    make check is happy on macOS.

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

  22. theuni commented at 10:04 pm on April 4, 2018: member
    very nice! concept ACK. Will review in detail.
  23. in src/util.cpp:422 in 78bc52bf86 
    421+static bool InterpretBool(const std::string& val)
422 {
423-    if (strValue.empty())
424-        return true;
425-    return (atoi(strValue) != 0);
426+    return val != "0";
    theuni commented at 10:18 pm on April 4, 2018:
    Looks like this will miss " 0" (whitespace), and “-0” (negative zero), which could happen as a typo on another argument.
  24. MarcoFalke commented at 8:55 pm on April 8, 2018: member
    Needs rebase
  25. laanwj commented at 7:34 am on April 25, 2018: member

    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.

  26. luke-jr commented at 2:49 am on May 2, 2018: member
    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
  27. promag commented at 0:37 am on May 30, 2018: member
    Needs rebase.
  28. ken2812221 commented at 2:14 pm on June 2, 2018: contributor
    Concept ACK
  29. MarcoFalke commented at 2:30 pm on June 2, 2018: member
    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.
  30. MarcoFalke closed this on Jun 2, 2018
  31. fanquake removed this from the "In progress" column in a project
  32. laanwj referenced this in commit 2dc5ab6378 on Jul 20, 2018
  33. meshcollider added the label Up for grabs on Oct 16, 2018
  34. meshcollider removed the label Up for grabs on Oct 16, 2018
  35. UdjinM6 referenced this in commit 0c5896078f on Jun 29, 2021
  36. UdjinM6 referenced this in commit db8ed9f495 on Jun 29, 2021
  37. UdjinM6 referenced this in commit 63443e26fd on Jul 1, 2021
  38. UdjinM6 referenced this in commit bec6b5e19f on Jul 2, 2021
  39. UdjinM6 referenced this in commit a9fb8a57ab on Jul 2, 2021
  40. DrahtBot locked this on Sep 8, 2021


eklitzke practicalswift Empact jnewbery ryanofsky theuni laanwj Sjors MarcoFalke luke-jr promag ken2812221


laanwj

Labels
Refactoring

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-05 01:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me