search

noloader / cryptopp-cmake

CMake files for Crypto++ project
BSD 3-Clause "New" or "Revised" License
92 stars 68 forks source link

When will it be updated to 8.7? #94

Closed Lveyou closed 2 years ago

Lveyou commented 2 years ago

thanks.

chausner commented 2 years ago

I'm also currently waiting for this wrapper to be updated for cryptopp 8.7.

abdes commented 2 years ago

The master branch is updated already. You can use it, no need to wait for a release tag.

chausner commented 2 years ago

The master branch is updated already. You can use it, no need to wait for a release tag.

Thank you for this tip. I am using the library through Conan. Before I can submit an update of the Conan port for 8.7, it is normally required to have a release tag that officially signals compatibility with the new release. Alternatively, it would be good to have at least the confirmation from a maintainer that it is indeed fine to use the current master branch with 8.7.

/cc @noloader

abdes commented 2 years ago

Understand. Could you please check the modernize branch and see if this new branch works for your scenario?

I'd like to update the project because it has many bugs but wanna make sure I'm not breaking too many use case.

You can continue to manually copy the CMakeLists.txt from the cryptopp folder, but I would not recommend that anymore. Instead, just follow the example in the https://github.com/noloader/cryptopp-cmake/blob/modernize/CMakeLists.txt for how to automatically clone crypto++ and setup the build in a clean way, compatible with the modern ways of cmake dependency management (ultimately, I will add a scenario just using FetchContent, or using CPM).

@chausner @Lveyou

chausner commented 2 years ago

I'd like to update the project because it has many bugs but wanna make sure I'm not breaking too many use case.

Cool! Although for my usecase, making the library available as a Conan package, I don't think git clones running as part of CMake are ideal. It would mean that everyone who wants to install the package needs to have git installed which is an unnecessary dependency since it could be easily replaced with a download of the source archive from GitHub, I assume? But these things can be patched/replaced in the Conan port later, if necessary, so feel free to modernize it as you see fit.

abdes commented 2 years ago

I'd like to update the project because it has many bugs but wanna make sure I'm not breaking too many use case.

Cool! Although for my usecase, making the library available as a Conan package, I don't think git clones running as part of CMake are ideal. It would mean that everyone who wants to install the package needs to have git installed which is an unnecessary dependency since it could be easily replaced with a download of the source archive from GitHub, I assume? But these things can be patched/replaced in the Conan port later, if necessary, so feel free to modernize it as you see fit.

Not a problem. I'll figure out a way to detect if git is available and if not, download and unpack the source code manually, or expect it to be in a certain location defined with a cmake option if the connection is not even possible.

I just wanna avoid the high maintenance cost of embedding crypto++ in the module as a submodule or a zip file, and allow people to also build from master not just release tags.

abdes commented 2 years ago

It's done. Here is the logic:

  1. The user can explicitly request a locally provided source directory for crypto++ using a cmake option: CRYPTOPP_SOURCES. If the location exists, it's great., otherwise the build fails.
  2. If that was not the case, the build script will check for cmake version (with (>=3.11) or without FetchContent) and prefers FetchContent. 2.1.- If git is available on the environment, we use git clone with FetchContent, 2.2 If git is not available, we use a URL to the release tag source package, unless the user requested to use the MASTER branch (with cmake option CRYPTOPP_USE_MASTER_BRANCH
  3. If FetchContent is not available, we fall back to a git clone only if git is available.

Of course if none of the above works, the build just fails.

I think this should cater for every possible scenario. If you need something different, just let me know.

Lveyou commented 2 years ago

Thanks for your improvement. I used visual studio 2022 preview, but the following error occurred: 1> [CMake] CMake Error at third/cryptopp-cmake/CMakeLists.txt:24 (include): 1> [CMake] include could not find requested file: 1> [CMake] 1> [CMake] GetGitRevisionDescription @abdes

Vollstrecker commented 2 years ago

I will add a scenario just using FetchContent, or using CPM).

You mean like in #67 where I did exactly that and failed because of....

Lveyou commented 2 years ago

I'm new to cmake and the error seems to be because I put cryptopp-cmake into a subdirectory. This is my project git address, which has the outermost cmakelist.txt: https://gitee.com/lveyou/dnd.git But now the project still uses cryptopp8.6, it would be great if it could simply integrate cryptopp8.7. @abdes @Vollstrecker

Vollstrecker commented 2 years ago

From my side, I already impleted in my PR all points @abdes listed except the fallback. The git method is slightly faster so I would prefer to use git unless it's not found and fall back to http then. But I have to admit that I maybe remember this false, as my PR was declined without telling me where the problems are which highly demotivated anyone working on this (there are many more pr's that just got stuck waiting for a clarification of the problem noone seems to see).

abdes commented 2 years ago

@Vollstrecker would you please check the example in the test case under: https://github.com/noloader/cryptopp-cmake/tree/master/test/standard-cpm ? That test case is actually the recommended way to use cryptopp-cmake with modern cmake version. With very old versions, you can just do like usual and put cryptopp-cmake as a submodule in your project, or any other way you used to do. The crypto++ fetching is now totally automated, for all version of cmake, unless of course you override it with a local copy of crypto++.

README has also been updated.

Your efforts and support are very appreciated. The project needed much more cleanup as bug fixing and I did not have too much time to spend on it, so I wanted to do everything while I can. I read your PR and I made sure all your ideas were integrated, as well as the comments you made in the subsequent discussion. Your contributions are always welcome.

abdes commented 2 years ago

@Lveyou I looked at your project. You just need to add cryptopp-cmake as a submodule in third, and remove cryptopp, which will be fetched and built automatically for you. Then just add-subdirectory(third/cryptopp-cmake) et voila.

The master branch already points to crypto++ 8.7.0, and I will soon release a new tag for 8.7.0 as well.

Vollstrecker commented 2 years ago

On a first read this looks (almost) OK, except the hardcoded CRYPTOPP_BUILD_TESTING. Maybe you know, I'm working on a slightly similar thing https://github.com/Vollstrecker/CmDaB where I prefer to let the user decide if and which tests of the deps are run.

I can't do a real test before Wednesday as I'm on a trip atm. and only have my phone here. For real work I hope I find time in the last quarter of the year, I even can't finish my work on upnp source-packaging since ~5 months.

abdes commented 2 years ago

I thought you can always override it from the command line or by setting the option before you add the cryptopp-cmake subdirectory. The value in the CMakeLists.txt is simply the default behavior if nothing is set by the caller.

If I'm missing something please suggest an improvement for it should be done.

Vollstrecker commented 2 years ago

I never used cpm, I will try on wednesday but https://github.com/noloader/cryptopp-cmake/blob/96f73bb12777e4679c12c22e761f6e95e6a9abfd/test/standard-cpm/CMakeLists.txt#L54 looks to me like it potentially could overwrite the setting.

abdes commented 2 years ago

I never used cpm, I will try on wednesday but

https://github.com/noloader/cryptopp-cmake/blob/96f73bb12777e4679c12c22e761f6e95e6a9abfd/test/standard-cpm/CMakeLists.txt#L54

looks to me like it potentially could overwrite the setting.

Yes. It does and it's made on purpose as the test case is simulating a project that uses cryptopp-cmake and is not interested in running its tests, a typical user project.

This only applies to the test case, and not to the project itself 😃 .

Lveyou commented 2 years ago

Very good, I successfully passed the compilation. Then I tried again:

option(CRYPTOPP_SOURCES
        "Use the provided location for crypto++ sources; do not fetch" "${PROJECT_SOURCE_DIR}/third/cryptopp")

In the end I found out that it should be written like this:

set(CRYPTOPP_SOURCES "${PROJECT_SOURCE_DIR}/third/cryptopp")
option(CRYPTOPP_SOURCES
        "Use the provided location for crypto++ sources; do not fetch" ON)

it worked successfully.