Skip to content
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

bump fc and fix node #1405

Merged
merged 3 commits into from
Oct 29, 2018
Merged

Conversation

oxarbitrage
Copy link
Member

While doing some development i bumped FC to the last version and when doing so I noticed that it will not build with error:

/home/alfredo/CLionProjects/lua/libraries/net/node.cpp: In constructor ‘graphene::net::detail::node_impl::node_impl(const string&)’:
/home/alfredo/CLionProjects/lua/libraries/net/node.cpp:320:11: error: ‘rand_pseudo_bytes’ is not a member of ‘fc’
       fc::rand_pseudo_bytes(&_node_id.data[0], (int)_node_id.size());

This is due to changes introduced here: bitshares/bitshares-fc#84

The same compiler provided a hint to fix it after the error msg:

/home/alfredo/CLionProjects/lua/libraries/net/node.cpp:320:11: note: suggested alternative: ‘rand_bytes’
       fc::rand_pseudo_bytes(&_node_id.data[0], (int)_node_id.size());
           ^~~~~~~~~~~~~~~~~
           rand_bytes

This pull request bumps FC and add the fix changing rand_pseudo_bytes to rand_bytes. I am honestly not totally sure about the implications but just know this way it will build.
Appreciate your comments to confirm if this is a good fix or if need to do it different. Alternative can be to bring back rand_pseudo_bytes or revert the FC pull fully(as changes were done only to fix some warnings)

jmjatlanta
jmjatlanta previously approved these changes Oct 26, 2018
Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

Yes, that is correct. The FC PR to fix the warnings also included the removal of deprecated methods. The difference between rand_pseudo_bytes and rand_bytes was small, and rand_pseudo_bytes was not recommended. Hence, the deprecation.

Compiled with Boost 1.67 / OpenSSL 1.10 / Ubuntu 18.04

cogutvalera
cogutvalera previously approved these changes Oct 27, 2018
Copy link
Member

@cogutvalera cogutvalera left a comment

Choose a reason for hiding this comment

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

We can merge it IMHO. Thanks !

@abitmore
Copy link
Member

Why docker cloud build is failing?

@cogutvalera
Copy link
Member

How to see docker cloud's logs ? After clicking on details link I see 404 error

@oxarbitrage
Copy link
Member Author

I think it timed out or something, in the logs i see "Waiting for logs..." and nothing more. The duration of the failed build was 2 hours, generally successful builds take an hour. I restarted now, lets see how it goes.

@cogutvalera
Copy link
Member

sometimes travis & docker cloud must be restarted and it will help. Travis I can restart (have permission), but docker cannot, maybe somebody may help me here to give such access too, Thanks !

@oxarbitrage
Copy link
Member Author

Travis is faster and have less issues in general, need to be restarted on each new pull request to populate the cache @pmconrad added to it, only fails when the pull is new.
Docker takes more time and can have issues when to many builds are done in a short period of time, we use it mainly because we have Dockerfile in the project, if not we could just use travis alone.

@cogutvalera you need to have a dockercloud account, then go to https://cloud.docker.com/u/bitshares to see the builds of bitshares-core.
I am not sure if just an account is enough or we have to ask @xeroc to add you to the organization as he was IIRC the creator.

@cogutvalera
Copy link
Member

cogutvalera commented Oct 27, 2018

@oxarbitrage Thanks ! I've docker cloud account, but BitShares builds are forbidden for me.

@pmconrad
Copy link
Contributor

It keeps running into "Build canceled" after about 50%. Perhaps compilation of database.cpp eats too much memory.

@pmconrad
Copy link
Contributor

Perhaps try cmake-option -D GRAPHENE_DISABLE_UNITY_BUILD=ON in Dockerfile.

from cmake command in dockerfile as it will not fix the dockercloud issue.
@oxarbitrage
Copy link
Member Author

Perhaps try cmake-option -D GRAPHENE_DISABLE_UNITY_BUILD=ON in Dockerfile.

tried but failed at the same place.

@oxarbitrage
Copy link
Member Author

This is unrelated to this particular code as cogutvalera:issue_1171 is failing with the same error.

I think we should try to fix it outside the context of this pull request to don't start adding several tests and unrelated commits to it.

@cogutvalera
Copy link
Member

#1171 (#1382 PR) must be rebased after merging this PR

Copy link
Contributor

@jmjatlanta jmjatlanta left a comment

Choose a reason for hiding this comment

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

UNITY stuff retracted. Approving again to verify that dockercloud does not error out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants