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

Bye Boost Lib 🙌 #252

Merged
merged 9 commits into from
Apr 6, 2022
Merged

Bye Boost Lib 🙌 #252

merged 9 commits into from
Apr 6, 2022

Conversation

mehah
Copy link
Owner

@mehah mehah commented Apr 5, 2022

well, the name says, many of the things we were using from the boost lib, already exist in c++20, which does not exist, equivalent methods were created or replaced by other libs, with that, we reduced the size of the executable and it will take less time to download the libs in vcpkg.

Note:
[32bits]
vcpkg install asio:x86-windows

[64bits]
vcpkg install asio:x64-windows

[static 32bits]
vcpkg install --triplet x86-windows-static asio

@mehah mehah requested review from scopz, dudantas and nekiro April 5, 2022 23:20
@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

Kinda sad that stduuid is not available from vcpkg :(

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

I think this PR breaks compilation on ubuntu 20.04, #include <span> which is used by uuid is not available under ubuntu 20.04
source: mgbowen/windows-fido-bridge#15

@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

Kinda sad that stduuid is not available from vcpkg :(

I opened an issue: mariusbancila/stduuid#63

I think this PR breaks compilation on ubuntu 20.04, #include <span> which is used by uuid is not available under ubuntu 20.04 source: mgbowen/windows-fido-bridge#15

there is a solution for this: mariusbancila/stduuid#60 (comment)
but our project is c++20, let's wait for @scopz to test it, since his is ubuntu 20

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

Kinda sad that stduuid is not available from vcpkg :(

I opened an issue: mariusbancila/stduuid#63

I think this PR breaks compilation on ubuntu 20.04, #include <span> which is used by uuid is not available under ubuntu 20.04 source: mgbowen/windows-fido-bridge#15

there is a solution for this: mariusbancila/stduuid#60 (comment) but our project is c++20, let's wait for @scopz to test it, since his is ubuntu 20

Well, we are not using stduuids cmake, cause you included the raw code into otclient, so that won't work, uuid.h includes from std library, and that's not present on ubuntu 20.04 (I tested compiling it there), so we probably need gsl to use instead (that's configured in cmake), that would mean we have to include gsl into otclient source too.

EDIT:
I got it working by updating g++ to version 10 which includes span, but do we want that hassle just to add this uuid library?

source:
https://askubuntu.com/questions/1263704/how-do-i-install-gcc-10-2-with-sudo-apt-get-install-gcc-x-g-x
https://tuxamito.com/wiki/index.php/Installing_newer_GCC_versions_in_Ubuntu

how to update g++:

sudo add-apt-repository -y ppa:ubuntu-toolchain-r/test
sudo apt update
sudo apt install -y g++-11
sudo update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-11 6
sudo update-alternatives --config gcc
gcc --v

compilation:
image

@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

EDIT: I got it working by updating g++ to version 10 which includes span, but do we want that hassle just to add this uuid library?

well i don't see the problem, we put a solve saying we need gcc 10+.

when stduuid goes to vcpkg, I remove it from the code, because I don't like it to be like that.

so can we proceed with the merger? 😀

@scopz
Copy link
Collaborator

scopz commented Apr 6, 2022

After
sudo apt-get install -y libasio-dev

it's building fine to me 👍🏼

scopz
scopz previously approved these changes Apr 6, 2022
@scopz
Copy link
Collaborator

scopz commented Apr 6, 2022

I assume, after these changes, there won't be any need to install libboost-all-dev, right?
We have to update wikis

@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

maybe we have to change cmake too.

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

If boost is completely not used, we can remove it

@scopz
Copy link
Collaborator

scopz commented Apr 6, 2022

What about

#include <boost/thread.hpp>

@scopz scopz self-requested a review April 6, 2022 18:27
@scopz scopz dismissed their stale review April 6, 2022 18:28

requires some changes yet ;)

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

What about

#include <boost/thread.hpp>

I think we could just get rid of this, its useless feature to have sql in client and I cant even see potential use for it XD
Visual Studio doesn't even compile this file, because its not added to it.

@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

I think we could just get rid of this, its useless feature to have sql in client and I cant even see potential use for it XD Visual Studio doesn't even compile this file, because its not added to it.

I agree, I'll remove it in the next cleanup-related PR I'm doing, besides, the mysql library only has it for x64.
microsoft/vcpkg#11214 (comment)

about cmake, can anyone remove the boost and test?

@scopz
Copy link
Collaborator

scopz commented Apr 6, 2022

I did comment the part using boost and it built correctly. I'm not in my pc anymore, so can't push it right now.

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

Hmm, using g++10 breaks compilation of json somehow
I'm having this problem:
nlohmann/json#2129

@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

Try gcc11 😁

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

Yep, g++11 does compile fine, 1:1 branch code as of now.
Actually no, removing boost spawns linking errors xD
EDIT: nvm, i have 12 cores and make -j breaks compilation, so I have to use make -j 6 XD

@nekiro
Copy link
Collaborator

nekiro commented Apr 6, 2022

Gotta update wiki after merge of this.

@mehah mehah merged commit 877a163 into main Apr 6, 2022
@mehah mehah deleted the eliminating_boost_lib branch April 6, 2022 22:44
@mehah
Copy link
Owner Author

mehah commented Apr 6, 2022

Gotta update wiki after merge of this.

I have already updated vcpkg libs on windows

@nekiro
Copy link
Collaborator

nekiro commented Apr 7, 2022

Gotta update wiki after merge of this.

I have already updated vcpkg libs on windows

ubuntu is updated too

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.

None yet

3 participants