NotificationsYou must be signed in to change notification settings
Fork425
Star2k

Expose OS-chosen address and port; force IPv4#707

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

eile wants to merge3 commits intocpp-netlib:0.11-develfromBlueBrain:0.11-bbp

Closed

Expose OS-chosen address and port; force IPv4#707

eile wants to merge3 commits intocpp-netlib:0.11-develfromBlueBrain:0.11-bbp

Conversation

Copy link

eile commentedNov 10, 2016

No description provided.

Expose OS-chosen address and port; force IPv4

b886faa

deanberris reviewed

Nov 14, 2016

View reviewed changes

Copy link

Member

deanberris left a comment

Choose a reason for hiding this comment

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

Can you explain why this is necessary, and whether it's possible for you to make these options instead of forcing the defaults?

CMakeLists.txt

		option( CPP-NETLIB_BUILD_SHARED_LIBS"Build cpp-netlib as shared libraries."ON )
		option( CPP-NETLIB_BUILD_TESTS"Build the cpp-netlib project tests."OFF)
		option( CPP-NETLIB_BUILD_EXPERIMENTS"Build the cpp-netlib project experiments."OFF)
		option( CPP-NETLIB_BUILD_EXAMPLES"Build the cpp-netlib project examples."OFF)

Copy link

Member

Choose a reason for hiding this comment

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

I'm not comfortable with changing these defaults. Is there a reason why you don't set these from your project?

boost/network/protocol/http/server/sync_server.hpp

		system::error_code error;
		tcp::resolverresolver(service_);
		tcp::resolver::queryquery(address_, port_);
		tcp::resolver::queryquery(tcp::v4(),address_, port_);

Copy link

Member

Choose a reason for hiding this comment

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

There seems to be no reason for this -- can you not make this explicit in your code instead? As part of the options?

Fix MSVC build

62269d2

Copy link

Member

deanberris commentedDec 15, 2016

Hi@eile -- I sent some comments a while back and I'm unclear as to why this is necessary and why this isn't an optional thing (as opposed to something that forces IPv4). Do you mind making it so that we can configure what kind of IP addresses we'd like to get from the DNS would be? Then I can go ahead and merge the updated version.

Copy link

Author

eile commentedDec 15, 2016

Hi@deanberris: @tribal-tec and me where a bit swamped lately, we'll get back to you on this.

Copy link

Member

deanberris commentedDec 15, 2016

All good, thanks@eile -- no rush. :)

Copy link

Member

deanberris commentedDec 15, 2016

Also by the way, I just noticed that you're attempting to make a change to the 0.11-devel branch, which we aren't actively maintaining anymore. We're trying to get everyone to use the 0.13-release branch instead. I'm happy to still merge an updated version of this in 0.11-devel, but I'm not really up for having another release on the 0.11.x line.