search

ygrek / mldonkey

cross-platform multi-network p2p daemon
http://mldonkey.sourceforge.net/
Other
235 stars 43 forks source link

Fix byte type collision in bundled crypto++ with C++17 #66

Open chusopr opened 3 years ago

chusopr commented 3 years ago

This fixes the issue mentioned in #62 that C++17 introduces a new byte type which conflicts with the one defined by Crypto++.

This was also discussed upstream: https://www.cryptopp.com/wiki/Std::byte

And this patch adopts the same solution as upstream, which is moving their byte definition to the CryptoPP namespace: https://github.com/weidai11/cryptopp/commit/00f9818b5d8e5aeb6c1057aa395317635bae07a3

Actually, this is basically the same patch.

This PR also reverts #63, as forcing C++14 wouldn't be required anymore and the patch submitted by #63 didn't work for me when I tried to apply it to the Gentoo package (downstream bug #790134).

(Edit: it was previously said this PR reverts #62, but the actual PR number is #63, which is linked to issue #62)

HinTak commented 3 years ago

FWIW, I am against bundling a patched version of cryptopp , and carrying a non-trivial patch as that. It is better to upgrade and sync with upstream, instead of patching the old version. My 2 cents.

62 doesn't do much more than just testing if the compiler takes "-std=c++14", and prepend if it does. If it does not work with gentoo, you proably want to check that you are starting from clean mldonkey. In particular, you need to do 'make distclean' or some such to remove the generated "config/configure", for it to be re-generated, if you have already started ./configure and building and encountered the error.

chusopr commented 3 years ago

FWIW, I am against bundling a patched version of cryptopp

I totally agree with that.

I don't really know why bundling a copy of crypto++ looked like a good idea. I know the situation may be different in other OS's (mostly, Windows), but Linux and other systems usually have a current version of crypto++ available that MLDonkey could be linked against.

Bundling its own copy of crypto++ would lead to falling behind upstream until issues starting to appear due to outdated unmaintained code. Which is exactly what is happening. This bundled crypto++ has been untouched for 7 years. And it's then when issues start appearing.

and carrying a non-trivial patch as that.

You mean my patch is non-trivial? It's just a change in less than ten lines to move a variable to a new namespace, which is exactly the same change done upstream after long discussion and testing.

It is better to upgrade and sync with upstream, instead of patching the old version. My 2 cents.

That's basically what this PR is doing by applying the same patch that was applied upstream to fix #62. What #63 does instead is sticking compilation to C++14 to avoid the issue without fixing it.

I would rather remove this bundled crypto++ version and link against the system's one, but that doesn't look like an easy change.

62 doesn't do much more than just testing if the compiler takes "-std=c++14", and prepend if it does. If it does not work with gentoo, you proably want to check that you are starting from clean mldonkey. In particular, you need to do 'make distclean' or some such to remove the generated "config/configure", for it to be re-generated, if you have already started ./configure and building and encountered the error.

No, I haven't already run ./configure and started the build process. The build process is automated in Gentoo and that's not how it works.

Anyway, I don't think it's worth discussing whether #63 works or not for keeping compilation at C++14 to avoid an issue that can be fixed instead the same way that was fixed upstream.

sl1pkn07 commented 1 year ago

Hi

please, can you remove the #include <caml/config.h> in the src/utils/lib/CryptoPP.h file ? this can resolve #86 when apply this PR

greetings