Properly work with USE_UPNP from makefiles #461

muggenhor commented at 12:08 PM on August 11, 2011: contributor

Use "#if USE_UPNP" instead of "#ifdef USE_UPNP" to work properly with the makefiles

Additionally, don't try to link with libminiupnc when USE_UPNP=0.

Signed-off-by: Giel van Schijndel me@mortis.eu

sgimenez commented at 12:58 PM on August 11, 2011: contributor

How to compile without linking to libupnp then?

muggenhor commented at 1:07 PM on August 11, 2011: contributor

Set USE_UPNP=0 in the makefile. (or on the commandline)

sgimenez commented at 2:20 PM on August 11, 2011: contributor

This does not sound consistent with the init.cpp change. There was three states before : undefined -> no linking, 0 -> linked but off by default. 1 -> linked and on by default. A bit odd, but a least functional.

TheBlueMatt commented at 5:39 PM on August 11, 2011: member

The relevant section from build-unix.txt:

Requires miniupnpc for UPnP port mapping. It can be downloaded from http://miniupnp.tuxfamily.org/files/. UPnP support is compiled in and turned off by default. Set USE_UPNP to a different value to control this: USE_UPNP= no UPnP support, miniupnp not required; USE_UPNP=0 (the default) UPnP support turned off by default at runtime; USE_UPNP=1 UPnP support turned on by default at runtime.

gavinandresen commented at 4:16 PM on August 12, 2011: contributor

I hate to open this kettle of worms...

... but the tri-state nature of USE_UPNP seems to be a chronic source of confusion. Any objections to changing the rules to be:

USE_UPNP=1 : upnp support compiled in and turned on by default at runtime Anything else: upnp support NOT compiled in.

And the build rules: bitcoin GUI : built with USE_UPNP=1 bitcoind : no upnp

TheBlueMatt commented at 4:19 PM on August 12, 2011: member

Well the original argument was we will have autotools in a couple weeks so it will all go away...but autotools kinda fell flat.

muggenhor commented at 11:52 PM on August 12, 2011: contributor

I'm not sure how autotools would fix that problem, as the tri-state nature of USE_UPNP would remain a problem... If you really want to be able to configure the default UPnP state at compile time (which seems rather senseless to me): use two macros for the job.

TheBlueMatt commented at 11:55 PM on August 12, 2011: member

Well autotools can figure out if upnp is there and then automatically either set or unset USE_UPNP and you can do a configure flag to set the default on or off.

muggenhor commented at 11:58 PM on August 12, 2011: contributor

The fact that upnp is available doesn't mean you actually want to build against it. As for a configure flag to set the default on/off, the same can be accomplished with a macro (the -D thing) by optionally defining one depending on make's command line....

TheBlueMatt commented at 12:01 AM on August 13, 2011: member

I cant see many scenarios where you would want to not build against a library you have even if its use if off by default (most programs will build this way, even if you can force it one way or another)... Anyway, not that it really matters, the initial idea was autotools could have more/more clear flags.

Use "#if USE_UPNP" instead of "#ifdef USE_UPNP" to work properly with the makefiles

Additionally, don't try to link with libminiupnc when USE_UPNP=0.

Signed-off-by: Giel van Schijndel <me@mortis.eu>

a897ead46f

in src/init.cpp:None in 79f9723f04 outdated

 183 |  #if USE_UPNP
 184 |              "  -noupnp          \t  "   + _("Don't attempt to use UPnP to map the listening port\n") +
 185 |  #else
 186 |              "  -upnp            \t  "   + _("Attempt to use UPnP to map the listening port\n") +
 187 |  #endif
 188 | -#endif

TheBlueMatt commented at 2:12 PM on August 13, 2011:

If you are going to redefine USE_UPNP to mean in and on by default, you should remove the whole #else -upnp (and its existence elsewhere in the code) block.

laanwj commented at 6:08 PM on October 11, 2011: member

I'm fine with it being possible to link to libupnp without enabling it by default (linux distributions might use this) but these would indeed be better off as separate options, the tristate option is really confusing.

T-X commented at 12:23 PM on November 2, 2011: none

I like gavin's suggestion to only have one option, USE_UPNP=1 or else.

Just one question, because I don't really know what is meant with "build rule": Is the build rule only refering to the provided, statically linked binaries on the website? Or to the default behaviour of qmake and make?

I think that as long as for instance a simple "make -f makefile.unix" after a clean git checkout does not build cleanly on all major Linux distros due to version conflicts between bitcoin and libminiupnp, libminiupnp should better not be selected by default. If autotools can figure out whether using libminiupnp is possible later, great. But until then, I'd vote for upnp off by default in both bitcoin-qt and bitcoind.

laanwj commented at 12:57 PM on November 2, 2011: member

how big is libupnpc? would it be feasible to include it in bitcoin like we do with spirit-json? if it is only a few source files we could... as it is not a very common library for distributions to have available

TheBlueMatt commented at 3:02 PM on November 2, 2011: member

@T_X in gavin's comment, build rule would refer to the default when calling make/qmake+make and the distributed binaries.

Personally, I don't care what the default is, the tri-state option was added because no one would agree on what the default for UPnP should be and thus it wasn't getting merged, so tri-state was added to move the process forward. If you want to change it, go ahead, but even two different arguments would be clearer than tri-state (--have-upnp and --upnp-by-default or something).

Distributing libminiupnpc? No way, we already distribute our own outdated, modified copy of json_spirit which we should never be doing. We don't need to be distributing other outdated copies of software in our src. Also, libminiupnpc is now in debian and gaining packages in other distros, so I'm not too concerned. Also, users who know enough to be compiling bitcoin from source probably aren't using UPnP on their router anyway, so most people who this effects are just going to disable UPnP anyway.

In any case, if someone wants to put in the effort and actually make an update to the way UPnP is defined, go right ahead...this pull request is outdated and only halfway done. It would be better to discuss updates specific to implementation instead of wandering around all day asking what we should do.

laanwj commented at 8:01 PM on November 2, 2011: member

IMO it would be fine to always enable libupnp by default when it is linked. After all, it can still be disabled through a command-line option and in the options UI.

But you're right that it seems that we can never agree on anything and so these kind of small changes linger forever.

neofutur commented at 9:44 PM on March 17, 2012: none

also have a problem with this USE_UPNP build option mostly not working, please fix

laanwj commented at 10:43 PM on March 17, 2012: member

It's not broken afaik.

Explain "mostly not working"?

neofutur commented at 6:09 AM on March 18, 2012: none

just tried to build from latest github and I had to manually modify the makefile to build with no UPNP libs or includes I ll try again, perhaps its just me.

sipa commented at 1:41 AM on March 19, 2012: member

@neofutur have you even read the rest of this issue? I'm sure it is confusing, and I agree it needs to be changed, but you know that USE_UPNP= disables linking against miniupnpc?

gavinandresen closed this on Apr 2, 2012

dexX7 referenced this in commit 36b61a22b8 on Jun 8, 2017

Losangelosgenetics referenced this in commit 57d2e2d3bf on Mar 12, 2020

DrahtBot locked this on Sep 8, 2021