Do not compile and install DHT_bootstrap if it was disabled in configure

[jin-eld]

closes #319

---

This change is 

[robinlinden]

Can you make this change in CMakeLists.txt too? I think most people compiling c-toxcore are using CMake for it.

[nurupo]

This PR is invalid as DHT_bootstrap and tox-bootstrapd (the DHT bootstrap daemon) are two separate programs. The thing you disable in configure is the daemon, not DHT_bootstrap.

[jin-eld]

If it is like @nurupo says, then the actual issue is #319 is invalid? I talked to @iphydf before implementing the fix, so I guess there was some confusion on all sides.

@robinlinden Regarding cmake - I can't, I am not familiar with it, I offered iphydf to help out with the autoconf bugs, if the same problem exists in the cmake build, then someone who is familiar with it should fix it too.

[iphydf]

@zetok as person filing the issue may have thoughts on this.

[iphydf]

So what do we do about this? @zetok, @nurupo.

[zetok]

@nurupo what is DHT_bootstrap supposed to do then? Is it needed in a default library install?

Given that there are no docs, the simple assumption is that DHT_bootstrap is not needed for anything and should be removed.

It doesn't really matter which switch would cause it to stop being build/installed when it's not needed, as long as there's such switch.

Right now there's no switch AFAIK.

[nurupo]

@nurupo what is DHT_bootstrap supposed to do then?

DHT_bootstrap is a simple DHT bootstrap node program which existed since early days of toxcore and was always built together with toxcore for as long as I can remember. It's not a new behavior of the default toxcore build/install.

Is it needed in a default library install?

I don't have any strong opinion on that matter. Many libraries produce executable binaries in their default build, many also do not. Maybe some other Tox developers have a strong opinion on that?

Given that there are no docs, the simple assumption is that DHT_bootstrap is not needed for anything and should be removed.

What "docs" you are talking about?

It doesn't really matter which switch would cause it to stop being build/installed when it's not needed, as long as there's such switch.

Sure, I don't mind it having a separate build switch, just don't lump it under the same switch as the daemon.

[jin-eld]

So from the above discussion: I'll add another switch for it to keep it separate from the daemon. Do we want to compile it by default (i.e. should the switch default be enabled or disabled)?

[iphydf]

I'd suggest adding a new flag and enabling it by default iff the actual bootstrap daemon is also enabled by default. For cmake, see CMakeLists.txt:448. Look for option(BOOTSTRAP_DAEMON. You can do a similar thing for DHT_bootstrap, if you like. That would be enabled by default.

[zetok]

@nurupo

Is it needed in a default library install?

I don't have any strong opinion on that matter. Many libraries produce executable binaries in their default build, many also do not. Maybe some other Tox developers have a strong opinion on that?

Question is, is it useful for anything? The yes/no answer to that question is also the answer to the question whether it is needed in a default install.

What "docs" you are talking about?

The ones that are missing. Any documentation on using/something DHT_bootstrap.

[nurupo]

and enabling it by default iff the actual bootstrap daemon is also enabled by default

People often confuse the two, the DHT_bootstrap and the bootstrap daemon, thinking that DHT_bootstrap is the daemon and asking the help for the DHT daemon, for whoever tries to help them to figure out later that they are not actually using the daemon. That's one of the reasons why I did't want a single flag which will enable both of them.

The ones that are missing. Any documentation on using/something DHT_bootstrap.

Yeah, there is no documentation on DHT_bootstrap aside from the help/usage example when you run it with no arguments. Anyone willing to make manpages for DHT_bootstrap and tox-bootstrapd?

[jin-eld]

Updated.

Added an option --disable-dht-bootstrap (default: enabled) as discussed above.
The --enable-daemon option stays unchanged.

[robinlinden]

It might be easier to remember the options if both are --enable-x instead of mixing disabling and enabling features. I'm fine with the default being to not build it though.

[jin-eld]

Usually you describe the opposite of the default, because that's what the user will be using. @iphydf suggested that it should be enabled, therefore the option is called --disable-....

I can change it if everyone agrees on something, so please decide and either merge it or let me know what to change it to.

[iphydf]

I think AC_ARG_ENABLE generates both --enable- and --disable- options, so it doesn't really matter. I have no opinion - @zetok and @nurupo decide.

[zetok]

I don't really care about the naming, as long as there is an option and it's documented.

@jin-eld could you add docs about the switch to the INSTALL.md under Advanced configure options ?

I don't think though that it should be enabled by default though, since it doesn't seem to have any use.

If the default is set to enable, "packaged" toxcore in Gentoo is going to have it disabled anyways – defaulting to enabled will just add a bit more work for me.

[jin-eld]

I think AC_ARG_ENABLE generates both --enable- and --disable- options, so it doesn't really matter. I have no opinion - @zetok and @nurupo decide.

This is correct, we are only talking about the help description here, sorry if it was not clear. That's what I meant, usually you pick the opposite as the help text description, because that's what the user will have to specify in order to change the default behavior.

@zetok: I would add it to the docs if I knew how to describe it :) Would the bellow be enough?

• --enable-dht-bootstrap build DHT bootstrap utility (default=disabled)

I really don't know what this utility is doing so I can't describe it any further, as @nurupo mentioned there is hardly any information about it. If you think that the above line is not enough, please help me out and write up something that I could paste into the INSTALL.md

PR updated:

• switched option default to "disabled"
• updated help text to --enable-dht-bootstrap
• updated INSTALL.md

[iphydf]

This looks done. @zetok @nurupo?

[zetok]

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[iphydf]

Looks fine. I believe @nurupo's concerns are addressed. This PR is ready to be merged (modulo coveralls, which is currently broken for C/C++).

[iphydf]

Coveralls is fixed (#377). Please rebase on master so the build is restarted.

[nurupo]

What does cmake do? Should we also backport this behaviour to our cmake configuration?

---

Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from Reviewable

[iphydf]

It's probably a good idea, yes.