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

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

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

chusopr
Copy link
Contributor

@chusopr chusopr commented Jul 8, 2021

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: weidai11/cryptopp@00f9818

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)

…aks CryptoPP 5.x"

This reverts commit d9394aa as forcing C++14 is no longer required.
@chusopr chusopr changed the title Cpp17 cryptopp byte Fix byte type collision in bundled crypto++ with C++17 Jul 8, 2021
@HinTak
Copy link
Contributor

HinTak commented Jul 8, 2021 via email

@chusopr
Copy link
Contributor Author

chusopr commented Jul 9, 2021

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
Copy link

sl1pkn07 commented May 1, 2023

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

@ygrek
Copy link
Owner

ygrek commented Jul 29, 2024

the proper way forward would be to upgrade bundled cryptopp (or better yet unbundle), but i don't have capacity for this now
this patch makes sense and will be naturally dropped when cryptopp is upgraded

CI on macos is still not nappy - https://github.com/ygrek/mldonkey/actions/runs/10138536812/job/28030381201#step:9:2133
the reason apparently is autoconf doing too much o_O

merging anyway

@ygrek ygrek merged commit 51c64f9 into ygrek:master Jul 29, 2024
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

4 participants