Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: remove BOOST_CPPFLAGS usage from bitcoin-tx #26086

Merged
merged 1 commit into from
Sep 16, 2022

Conversation

fanquake
Copy link
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.

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.
@theuni
Copy link
Member

theuni commented Sep 14, 2022

Nice. ACK f839697.

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: theuni@e131d8f

@hebasto
Copy link
Member

hebasto commented Sep 15, 2022

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.

@fanquake
Copy link
Member Author

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.

@hebasto
Copy link
Member

hebasto commented Sep 15, 2022

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.

@fanquake
Copy link
Member Author

Since boost is our only remaining external consensus dependency, as a follow-up, I'm wondering if we need a less hackish version of: theuni@e131d8f

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).

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -15,7 +15,6 @@
#include <key_io.h>
#include <fs.h>
#include <policy/policy.h>
#include <policy/rbf.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iwyu reports:

bitcoin-tx.cpp should remove these lines:
- #include <policy/rbf.h>  // lines 18-18
- #include <util/translation.h>  // lines 29-29

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to leave this as is.

@fanquake fanquake merged commit 3d892d8 into bitcoin:master Sep 16, 2022
@fanquake fanquake deleted the bitcoin_tx_prune_boost_cpp branch September 16, 2022 11:12
@fanquake
Copy link
Member Author

Since boost is our only remaining external consensus dependency, as a follow-up, I'm wondering if we need a less hackish version of: theuni@e131d8f

@theuni did you want to follow up to this with a PR?

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 20, 2022
fanquake added a commit to fanquake/bitcoin that referenced this pull request Feb 8, 2023
@bitcoin bitcoin locked and limited conversation to collaborators Sep 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants