build: remove BOOST_CPPFLAGS usage from bitcoin-tx #26086

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:

   0bitcoin-tx.cpp should remove these lines:
   1- #include <policy/rbf.h>  // lines 18-18
   2- #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
fanquake theuni hebasto

Labels
Build system