build: remove BOOST_CPPFLAGS usage from bitcoin-tx #26086

pull fanquake wants to merge 1 commits into bitcoin:master from fanquake:bitcoin_tx_prune_boost_cpp changing 2 files +1 −2
  1. fanquake commented at 8:32 AM on September 14, 2022: member

    The only reason BOOST_CPPFLAGS was needed here, is because of the policy/rbf.h include, which ultimately includes boost multi_index via txmempool.h. However this include is unused.

  2. build: remove BOOST_CPPFLAGS usage from bitcoin-tx
    The only reason BOOST_CPPFLAGS is needed here, is because of the
policy/rbf.h include, which ultimately includes boost multi_index
via txmempool.h. However this include is actually unused.
    f839697d9b
  3. DrahtBot added the label Build system on Sep 14, 2022
  4. theuni commented at 4:32 PM on September 14, 2022: member

    Nice. ACK f839697d9b89002d020997154e182ddb4dccf45b.

    As an aside, do any of our c-i runs actually test this? At least for depends, the boost headers are installed to the same prefix as everything else, meaning that $(BOOST_CPPFLAGS) is almost always redundant.

    Since boost is our only remaining external consensus dependency, as a follow-up, I'm wondering if we need a less hackish version of: https://github.com/theuni/bitcoin/commit/e131d8f1e33baa9a9499d2e1a4a99b171566c5a5

  5. hebasto commented at 8:30 AM on September 15, 2022: member

    As an aside, do any of our c-i runs actually test this? At least for depends, the boost headers are installed to the same prefix as everything else, meaning that $(BOOST_CPPFLAGS) is almost always redundant.

    Agree. Right now a build will success even after sed 's/ $(BOOST_CPPFLAGS)//g' through the entire code base. To make manipulations with $(BOOST_CPPFLAGS) verifiable, I strongly believe that @theuni's suggestion should go first.

  6. fanquake commented at 8:39 AM on September 15, 2022: member

    As an aside, do any of our c-i runs actually test this? At least for depends, the boost headers are installed to the same prefix as everything else, meaning that $(BOOST_CPPFLAGS) is almost always redundant.

    At the moment, atleast a native macOS (M1) build will blow up, because (brew installed) Boost is in a "non-standard" path.

    Agree. Right now a build will success even after sed 's/ $(BOOST_CPPFLAGS)//g' through the entire code base.

    This isn't correct. Non-depends builds can/will fail.

  7. hebasto commented at 8:52 AM on September 15, 2022: member

    As an aside, do any of our c-i runs actually test this? At least for depends, the boost headers are installed to the same prefix as everything else, meaning that $(BOOST_CPPFLAGS) is almost always redundant.

    At the moment, atleast a native macOS (M1) build will blow up, because (brew installed) Boost is in a "non-standard" path.

    Agree. Right now a build will success even after sed 's/ $(BOOST_CPPFLAGS)//g' through the entire code base.

    This isn't correct. Non-depends builds can/will fail.

    Agree, but it appears that the only system on which a build will fail is macOS M1 due to the reasons mentioned above.

  8. fanquake commented at 9:16 AM on September 15, 2022: member

    Since boost is our only remaining external consensus dependency, as a follow-up, I'm wondering if we need a less hackish version of: https://github.com/theuni/bitcoin/commit/e131d8f1e33baa9a9499d2e1a4a99b171566c5a5

    Agree, but it appears that the only system on which a build will fail is macOS M1 due to the reasons mentioned above.

    I'm happy for us to move in the direction suggested by Cory as a follow up. I think the current set of cpp changes (including the other PRs) are straightforward, and can be reviewed without changing depends (I'm also checking flag usage locally).

  9. in src/bitcoin-tx.cpp:18 in f839697d9b 
      14 | @@ -15,7 +15,6 @@
  15 |  #include <key_io.h>
  16 |  #include <fs.h>
  17 |  #include <policy/policy.h>
  18 | -#include <policy/rbf.h>
    hebasto commented at 10:35 AM on September 15, 2022:

    iwyu reports:

    bitcoin-tx.cpp should remove these lines:
- #include <policy/rbf.h>  // lines 18-18
- #include <util/translation.h>  // lines 29-29
    fanquake commented at 11:07 AM on September 16, 2022:

    Going to leave this as is.

  10. hebasto approved
  11. hebasto commented at 10:36 AM on September 15, 2022: member

    ACK f839697d9b89002d020997154e182ddb4dccf45b, tested on Linux x86_64 using theuni's patch with depends.

  12. fanquake merged this on Sep 16, 2022
  13. fanquake closed this on Sep 16, 2022
  14. fanquake deleted the branch on Sep 16, 2022
  15. fanquake commented at 2:18 PM on September 16, 2022: member

    Since boost is our only remaining external consensus dependency, as a follow-up, I'm wondering if we need a less hackish version of: https://github.com/theuni/bitcoin/commit/e131d8f1e33baa9a9499d2e1a4a99b171566c5a5 @theuni did you want to follow up to this with a PR?

  16. sidhujag referenced this in commit 1e0bda0257 on Sep 20, 2022
  17. fanquake referenced this in commit f8b68c1f63 on Feb 8, 2023
  18. bitcoin locked this on Sep 16, 2023
Contributors
fanquaketheunihebasto
Labels
Build system
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: 2026-04-18 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me