Include tinyformat as a subtree #13845

pull Empact wants to merge 3 commits into bitcoin:master from Empact:tinyformat-subtree changing 24 files +2342 −1058
  1. Empact commented at 4:03 am on August 2, 2018: member

    This adds the existing c42f/tinyformat@689695c of tinyformat as a subtree, and extracts the modifications that had been made to a utilstrprintf.h wrapper file which is included instead of tinyformat.h directly.

    The benefits being that this should be easier to verifiably update and clearer what changes are being introduced, while making it easier to identify tinyformat.h as a dependency rather than a project file, and apply special treatment relative to that fact.

    This re-opens #13842

  2. fanquake added the label Upstream on Aug 2, 2018
  3. DrahtBot commented at 5:27 am on August 2, 2018: member
  4. laanwj commented at 8:11 am on August 2, 2018: member
    Concept ACK I didn’t do this initially because I didn’t expect tinyformat to be an actually maintained library (as well as not really understanding submodules very well at the time), but in retrospect this is better.
  5. scravy commented at 9:03 am on August 2, 2018: contributor
    utACK – LGTM
  6. laanwj added this to the milestone 0.18.0 on Aug 2, 2018
  7. laanwj commented at 2:08 pm on August 2, 2018: member
    labeling this for after the 0.18 branch-off, I don’t think this makes sense to do last-minute for 0.17
  8. Empact commented at 2:20 pm on August 2, 2018: member
    @laanwj Agreed
  9. Empact commented at 5:51 pm on August 2, 2018: member

    Repeating @MarcoFalke’s concern here for visibility:

    Seems like a lot of files to pull in just to make it a subtree, no? Imo tinyformat is really meant to be a single file just to be copied into the project." #13842 (comment)

    I have a mild preference for automatic checks of consistency with upstream, but #13846 exists as an alternative.

  10. Empact force-pushed on Aug 2, 2018
  11. in src/Makefile.am:24 in f5ee845d42 outdated
    20@@ -21,7 +21,7 @@ endif
    21 
    22 BITCOIN_INCLUDES=-I$(builddir) $(BDB_CPPFLAGS) $(BOOST_CPPFLAGS) $(LEVELDB_CPPFLAGS) $(CRYPTO_CFLAGS) $(SSL_CFLAGS)
    23 
    24-BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include
    25+BITCOIN_INCLUDES += -I$(srcdir)/secp256k1/include -I$(srcdir)/tinyformat
    


    ajtowns commented at 6:55 am on August 7, 2018:
    tinyformat.h is only included directly from utilstrprintf.h; seems more sensible to just have utilstrprint.h #include <tinyformat/tinyformat.h> and not extend the include path?
  12. ajtowns commented at 7:07 am on August 7, 2018: member
    Concept ACK
  13. laanwj commented at 12:49 pm on August 7, 2018: member

    Seems like a lot of files to pull in just to make it a subtree, no? Imo tinyformat is really meant to be a single file just to be copied into the project."

    After seeing the number of files added here I think I’m starting to agree with @MarcoFalke.

    Meta-nit: In general it’s a bad idea to open multiple competing PRs, because it distributes the discussion over multiple places, which leads to confusion.

  14. Empact commented at 4:09 pm on August 7, 2018: member

    Here are the files included in the subtree - 10 in total: https://github.com/Empact/bitcoin/tree/tinyformat-subtree/src/tinyformat

    #13846 cuts this to 2 1: https://github.com/Empact/bitcoin/tree/tinyformat-subdir/src/tinyformat

    The other changed files relate mostly to redirecting tinyformat.h includes to utilstrprintf.h.

  15. Empact force-pushed on Aug 7, 2018
  16. ajtowns commented at 4:54 am on August 8, 2018: member

    The two PRs is a bit of a pain. I think I have a mild preference for the 10-files-but-automatic-checks over 2-files-but-manual-checks, but am not really bothered either way.

    The 46-files-changed is mostly just churn to pull out the customisations into utilstrprintf.h. You could avoid that by renaming utilstrprintf.h back to tinyformat.h (which then includes tinyformat/tinyformat.h). If I’m counting right, doing that gets it down to 24 changes: 10 src/tinyformat/, src/tinyformat.h, dev-notes.md, copyright_header.py, Makefile.am, src/Makefile.am, 6 lint files, travis.yml, and src/primitives/block.{h,cpp}. The block.{h,cpp} changes are just to directly include some standard headers so could also be skipped.

  17. in src/Makefile.am:185 in 7823f5cff3 outdated
    181@@ -182,6 +182,7 @@ BITCOIN_CORE_H = \
    182   util.h \
    183   utilmemory.h \
    184   utilmoneystr.h \
    185+  utilstrprintf.h \
    


    ajtowns commented at 4:54 am on August 8, 2018:
    Is this change needed/useful?
  18. Empact commented at 5:17 am on August 8, 2018: member
    One option is to split this into 2 prs: the extraction of utilstrprintf.h and the move to a subdir (which would be pretty trivial at that point).
  19. ken2812221 commented at 11:48 am on August 12, 2018: contributor
    Agree with @ajtowns, rename utilstrprintf.h to tinyformat.h
  20. Empact force-pushed on Aug 12, 2018
  21. Empact force-pushed on Aug 12, 2018
  22. DrahtBot added the label Needs rebase on Aug 27, 2018
  23. Squashed 'src/tinyformat/' content from commit 689695cf5
    git-subtree-dir: src/tinyformat
    git-subtree-split: 689695cf58700e6defe3741829564cd682d5ae57
    ddbf92320a
  24. Merge commit 'ddbf92320a97efa36994f42144960094e3bd10e6' as 'src/tinyformat' 7605cd180b
  25. Drop src/tinyformat.h in favor of the subtree src/tinyformat/
    Introduce modifications to tinyformat via src/tinyformat.h
    
    This includes changes in:
    695041e4952ea40e0 - util: Update tinyformat
    1b8fd35aadfad6a1e - Make tinyformat errors raise an exception instead of assert()ing
    9b6d4c5cdc1ad7b12 - Move strprintf define to tinyformat.h
    6e5fd003e04b81115 - Move `*Version()` functions to version.h/cpp
    9eaa0afa6ec5d3dd0 - tinyformat: force USE_VARIADIC_TEMPLATES
    b651270cd6bfdd6d7 - util: Throw tinyformat::format_error on formatting error
    
    This does not include changes below, and it protects against future unintended
    modification of upstream code via git-subtree-check:
    64fb0ac016c7fd01c - Declare single-argument (non-converting) constructors "explicit"
    4d9b4256d89d1f7c6 - Fix typos
    
    Note I excluded only the tinyformat cpp from the boost lint checks out of an
    abundance of caution - these files are not actually built into bitcoin so are
    not at risk of pulling in unwanted dependencies, whereas the header, and other
    dependencies might be.
    8bb7c2519a
  26. Empact force-pushed on Aug 28, 2018
  27. Empact commented at 1:25 am on August 28, 2018: member
    Rebased for #13863 (actually, regenerated subtree and cherry-picked changes)
  28. DrahtBot removed the label Needs rebase on Aug 28, 2018
  29. laanwj commented at 1:37 pm on August 31, 2018: member
    Closing (see #13846 (comment))
  30. laanwj closed this on Aug 31, 2018

  31. MarcoFalke locked this on Sep 8, 2021

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-12-18 21:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me