build: Remove port-forwarding runtime setting options from configure #26896
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
fanquake
force-pushed
the
remove_network_compile_time_default_setting
branch
from
1489586 to
e9db104
Compare
fanquake
force-pushed
the
remove_network_compile_time_default_setting
branch
2 times, most recently
from
099b893 to
387b86a
Compare
fanquake
force-pushed
the
remove_network_compile_time_default_setting
branch
from
387b86a to
fb8f847
Compare
Contributor
Contributor
|
Concept ACK Looking at the history, the Dbus and qrencode are now handled in "special" qt configure checks, while the ipv6 option was removed in #4115. Control of ipv6 usage was still somewhat retained through the |
Member
|
Concept ACK on:
That's true. |
hebasto
approved these changes
Jan 28, 2023
Member
hebasto
left a comment
hebasto
left a comment
There was a problem hiding this comment.
ACK fb8f847
The content of src/config/bitcoin-config.h:
- on master (6b7ccb9)
$ grep -B 2 -E USE_UPNP\|USE_NATPMP src/config/bitcoin-config.h
/* NAT-PMP support not compiled if undefined, otherwise value (0 or 1)
determines default state */
#define USE_NATPMP 0
--
/* UPnP support not compiled if undefined, otherwise value (0 or 1) determines
default state */
#define USE_UPNP 0
- this PR:
$ grep -B 2 -E USE_UPNP\|USE_NATPMP src/config/bitcoin-config.h
/* NAT-PMP support */
#define USE_NATPMP 1
--
/* UPnP support */
#define USE_UPNP 1
Two suggestions only:
- No need to use
AC_DEFINE_UNQUOTEDmacros. TheAC_DEFINEworks just fine as no arguments require expansion. - Maybe document the value, which a macro is defined to, explicitly?
Default to false.
Default to false.
fanquake
force-pushed
the
remove_network_compile_time_default_setting
branch
from
fb8f847 to
d51f0fa
Compare
sedited
approved these changes
Jan 29, 2023
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Jan 30, 2023
…tions from configure d51f0fa doc: add release notes for 26896 (fanquake) 2b24879 build: remove --enable-upnp-default from configure (fanquake) 02f5a5e build: remove --enable-natpmp-default from configure (fanquake) 25a0e8b Remove configure-time setting of DEFAULT_UPNP (fanquake) 06562e5 Remove configure-time setting of DEFAULT_NATPMP (fanquake) Pull request description: This PR removes the `--enable-upnp-default` and `--enable-natpmp-default` options from configure. It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default. I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a `.conf` file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code? ACKs for top commit: hebasto: ACK d51f0fa, rebased and comments have been addressed since my recent [review](bitcoin#26896 (review)). TheCharlatan: ACK d51f0fa Tree-SHA512: 481decd8bddd8b03b7319591e3acf189f7b6b96c9a9a8c5bc1a3f8ec00d0b8f9b52d2f5c28a298a2ec947cfe9611cfd184e393ccb2e4e21bfce86ca7d4de60d3
5 tasks
PastaPastaPasta
pushed a commit
to knst/dash
that referenced
this pull request
Dec 4, 2023
…tions from configure d51f0fa doc: add release notes for 26896 (fanquake) 2b24879 build: remove --enable-upnp-default from configure (fanquake) 02f5a5e build: remove --enable-natpmp-default from configure (fanquake) 25a0e8b Remove configure-time setting of DEFAULT_UPNP (fanquake) 06562e5 Remove configure-time setting of DEFAULT_NATPMP (fanquake) Pull request description: This PR removes the `--enable-upnp-default` and `--enable-natpmp-default` options from configure. It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default. I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a `.conf` file, can't or don't want to pass command line options at runtime, and also don't want to modify the source code? ACKs for top commit: hebasto: ACK d51f0fa, rebased and comments have been addressed since my recent [review](bitcoin#26896 (review)). TheCharlatan: ACK d51f0fa Tree-SHA512: 481decd8bddd8b03b7319591e3acf189f7b6b96c9a9a8c5bc1a3f8ec00d0b8f9b52d2f5c28a298a2ec947cfe9611cfd184e393ccb2e4e21bfce86ca7d4de60d3
PastaPastaPasta
added a commit
to dashpay/dash
that referenced
this pull request
Dec 4, 2023
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR removes the
--enable-upnp-defaultand--enable-natpmp-defaultoptions from configure.It's odd to me that we maintain configure-time options for setting the default port-forwarding runtime state (but no other similar options), and I'm not sure what use-case it satisfies, that can't be achieved by multiple other means. I also doubt that we'll ever restart using these in release builds, or turning on any of this by default.
I think the only scenario these options would be used is when you want to compile your own binaries (we don't use them in Guix), with port-forwarding on by default, but otherwise can't or don't want to use a
.conffile, can't or don't want to pass command line options at runtime, and also don't want to modify the source code?