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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for Windows build (cli_test and socket.h) #1305

Merged
merged 4 commits into from Sep 14, 2018

Conversation

Projects
3 participants
@jmjatlanta

jmjatlanta commented Sep 3, 2018

This is a potential fix for Issue #1292

It is untested, as I do not have a working Windows build environment. I formalized this into a PR to make it easier to test.

Note: This PR is (currently) based on the develop branch.

@ryanRfox ryanRfox added this to In progress in Feature release (201810) via automation Sep 3, 2018

@ryanRfox

This comment has been minimized.

Member

ryanRfox commented Sep 3, 2018

@abitmore are you willing to test this one? We may want to add the 0 Help Wanted label to attract other Windows build testers.

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 3, 2018

I feel it's better to reuse related code in FC but not reinvent wheels here. For example, since the server will bind to an available port anyway, can we just return it after the bind to avoid calling bind twice?

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Sep 5, 2018

@abitmore says:

can we just return it after the bind to avoid calling bind twice?

We have the Application class, and in the real world we pass in boost::program_options for what port to bind to for P2P communication as well as API connections. If we do not specify these parameters, I believe they do not bind to ports. Are you saying we should pass parameters to the Application class to tell it to select its own unused port, and then we can later query for which ports it binds to? I know that you know that such a change is out of the scope of this fix, so I must be misunderstanding your question.

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 5, 2018

If we do not specify these parameters, I believe they do not bind to ports. Are you saying we should pass parameters to the Application class to tell it to select its own unused port, and then we can later query for which ports it binds to?

  • the application class knows the final port anyway
    • if a port parameter is passed in, it will use the port
    • if no port is passed in, it will either not bind, or bind to a random available port
  • for testing, I guess it's not hard to query that port from application, including null or so if it didn't bind to any port.

I know that you know that such a change is out of the scope of this fix, so I must be misunderstanding your question.

Perhaps it's out of scope, but I still think it's better to have it.

Thanks.

@jmjatlanta

This comment has been minimized.

jmjatlanta commented Sep 14, 2018

if no port is passed in, it will either not bind, or bind to a random available port

Let me see if I am clear:

For P2P, if we don't specify a port, it binds to a random available port. Querying afterward makes sense, and is already implemented.

For the rpc endpoint, the user has the option to pass in a port number. In the "real world" they will either (1) not want one, or (2) have one in mind so that they can connect their clients to it. In the "testing world" we just want a random port, and don't need to share that port number with anyone outside the test.

I believe what you are proposing is for tests to set the parameter "rpc-endpoint" to something like "127.0.0.1:0" and let the websocket server select an available port. We can then provide an application member method to query the websocket server to reveal the port selected.

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 14, 2018

Thanks for the explanation. I guess I was wrong (was thinking about p2p port while the function is about rpc port). To be clear, in the real world, I don't want the feature to bind to a random rpc port, so it doesn't make sense to have it just for test (even if we have it, we still need to test specified port).

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 14, 2018

   close(socket_fd);
tests\cli\main.cpp(105): error C3861: 'close': identifier not found

Need to use closesocket() for Windows.

@abitmore

This comment has been minimized.

Member

abitmore commented Sep 14, 2018

boost/test/unit_test_suite_impl.hpp(343): error C2248: 'MyInitialization::~MyInitialization': cannot access private member declared in class 'MyInitialization'
@abitmore

This comment has been minimized.

Member

abitmore commented Sep 14, 2018

error C2248: 'MyInitialization::~MyInitialization': cannot access private member declared in class 'MyInitialization'

Update: moving the WSA initialization/cleanup code to cli_fixture struct fixes the issue.

In additional, /bigobj is required for cli_test

Fixed Windows build for cli_test
- BOOST_GLOBAL_FIXTURE() is somehow unusable, so moved WSA initialization and cleanup to cli_fixture
- Added /bigobj

Feature release (201810) automation moved this from In progress to In Testing Sep 14, 2018

@abitmore abitmore merged commit 5a003be into develop Sep 14, 2018

2 of 3 checks passed

ci/dockercloud Your tests are pending in Docker Cloud
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Feature release (201810) automation moved this from In Testing to Done Sep 14, 2018

@abitmore abitmore deleted the cli_win branch Oct 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment