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

add check for maximum boost version #231

Closed
wants to merge 1 commit into from

Conversation

xloem
Copy link

@xloem xloem commented Jan 23, 2021

No description provided.

@xloem
Copy link
Author

xloem commented Jan 23, 2021

I added this to make bitshares-core build more easily. Maybe the check should go in bitshares-core, I'm not sure.

@xloem xloem marked this pull request as draft January 23, 2021 19:31
@xloem xloem force-pushed the boost-max-version branch from 67bafee to 7c4b25e Compare January 23, 2021 19:35
@xloem xloem marked this pull request as ready for review January 23, 2021 19:37
@abitmore
Copy link
Member

I'm not sure if it's a good idea to do so, since we always look for supporting latest boost library although often we're a bit late. bitshares/bitshares-core#2216

@xloem
Copy link
Author

xloem commented Jan 23, 2021

This PR would make sense if you do another release before supporting the latest version of boost.

@abitmore
Copy link
Member

We does support boost 1.70 and probably some higher versions. See bitshares/bitshares-core#2208. But the documentations are outdated. Fixing the documentations and adding support for higher boost versions are more productive IMHO.

@xloem
Copy link
Author

xloem commented Jan 24, 2021

? Doesn't bitshares/bitshares-core#2208 say 1.70 is still on the backburner for bitshares/bitshares-core#2216 ? I'm still running into bitshares/bitshares-core#1935 when I try to use boost 1.70

[edit: with bitshares-core, of course. I haven't tried building bitshares-fc in any standalone way]

maybe it would be helpful if I changed the PR to emit a warning instead of breaking the build or something

@abitmore
Copy link
Member

You're on Windows? It seems #150 only fixed non-Windows build.

@abitmore
Copy link
Member

Yeah, a warning would be better. Thanks.

@xloem
Copy link
Author

xloem commented Jan 24, 2021

I'm actually on RHEL7 with a manually built boost. I'm looking at the cause of the error.

I'm thinking the correct solution to this error would be to move the declaration of impl classes into header files, so the compiler can calculate their size itself. The problem is that compilers are allowed to arbitrarily pad classes for alignment or runtime debugging information or whatnot, so hardcoding their size would be advised against. What do you think of that idea?

I'm going to try coding up a quicker and simpler fix that changes the hardcoded number to a sum of sizeof() calls, just for this one instance, and submit that as a PR unless I hear more from you.

I see this PR is not the correct solution here.

@xloem xloem closed this Jan 24, 2021
@xloem
Copy link
Author

xloem commented Jan 24, 2021

bitshares-core/libraries/fc/include/fc/fwd_impl.hpp: In instantiation of ‘void fc::check_size() [with long unsigned int RequiredSize = 80; long unsigned int ProvidedSize = 72]’:
bitshares-core/libraries/fc/include/fc/fwd_impl.hpp:85:43:   required from ‘fc::fwd<T, S, Align>::fwd() [with T = fc::tcp_socket::impl; unsigned int S = 72; Align = double]’
bitshares-core/libraries/fc/src/network/tcp_socket.cpp:98:26:   required from here

@xloem
Copy link
Author

xloem commented Jan 24, 2021

To close this out, wiping my build folder resolved this issue for me.

If the sizeof approach is ever of interest, here was the change I had prepared. It's just the size of a pointer for the vtable if there are virtual functions, plus the size of every member:

diff --git a/include/fc/network/tcp_socket.hpp b/include/fc/network/tcp_socket.hpp
index 6e69f80..4f38da0 100644
--- a/include/fc/network/tcp_socket.hpp
+++ b/include/fc/network/tcp_socket.hpp
@@ -3,6 +3,9 @@
 #include <fc/io/iostream.hpp>
 #include <fc/time.hpp>
 
+#include <fc/asio.hpp>
+#include <fc/thread/future.hpp>
+
 namespace fc {
   namespace ip { class endpoint; }
 
@@ -52,11 +55,7 @@ namespace fc {
     private:
       friend class tcp_server;
       class impl;
-      #ifdef _WIN64
-      fc::fwd<impl,0xa8> my;
-      #else
-      fc::fwd<impl,0x60> my;
-      #endif
+      fc::fwd<impl,sizeof(void* /*vtable*/)+sizeof(fc::future<size_t>)*2+sizeof(boost::asio::ip::tcp::socket)+sizeof(tcp_socket_io_hooks*)> my;
   };
   typedef std::shared_ptr<tcp_socket> tcp_socket_ptr;

@abitmore
Copy link
Member

abitmore commented Jan 24, 2021

Yes, the sizeof approach is in my mind and it should fix the issue so we don't need to modify the value every time in the future. I just haven't got time to try it out. If your code works please feel free to create a PR. Thanks.

Update:
I think the impl class declared here was meant to hide the implementation details, perhaps for smaller binary file or whatever. But it seems the sizeof calculation is not serving the same purpose? What do you think?

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.

2 participants