Skip to content

Conversation

@fanquake
Copy link
Member

Their inclusion is likely just the result of copy-paste.

The only place upnp & natpmp CPPFLAGS should be used is libbitcoin_node (mapport.cpp).

@theuni
Copy link
Member

theuni commented Sep 14, 2022

ACK 557ed5973329052418ad371ebc43a74ac8096f1b. Confirmed that mapport.cpp is the only user of these includes.

From a quick glance it looks like these two shouldn't require libevent either though?

@hebasto
Copy link
Member

hebasto commented Sep 14, 2022

Concept ACK.

Their inclusion is likely just the result of copy-paste.

That is true at least for NATPMP_CPPFLAGS.

Their inclusion is likely just the result of copy-paste.

The only place upnp/natpmpflags  should be used is `libbitcoin_node`
(mapport.cpp).
@fanquake fanquake changed the title build: remove unused natpmp / upnp cppflags build: remove unused cppflags Sep 15, 2022
@fanquake fanquake force-pushed the prune_unneeded_upnp_natpmp branch from 557ed59 to 4b656b9 Compare September 15, 2022 08:57
@fanquake
Copy link
Member Author

From a quick glance it looks like these two shouldn't require libevent either though?

Looks like it. Have added a commit to drop them as well.

Copy link
Member

@theuni theuni left a comment

Choose a reason for hiding this comment

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

ACK 4b656b9

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 4b656b9, I have reviewed the code and it looks OK, I agree it can be merged.

@fanquake fanquake merged commit 20f03a5 into bitcoin:master Sep 15, 2022
@fanquake fanquake deleted the prune_unneeded_upnp_natpmp branch September 15, 2022 14:24
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 15, 2022
@bitcoin bitcoin locked and limited conversation to collaborators Sep 15, 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.

3 participants