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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CMake build #77

Merged
merged 1 commit into from Dec 25, 2015
Merged

CMake build #77

merged 1 commit into from Dec 25, 2015

Conversation

GamePad64
Copy link
Contributor

Hello,
I have added CMake build, as mentioned in #73.

Building static+shared library.

  • Honors object linking order, to contend Static Initialization Order Fiasco.
  • Forces adding -fPIC to all 64-bit architectures, such as amd64 and aarch64. For 32-bit architectures behaviour is controlled using -DCMAKE_POSITION_INDEPENDENT_CODE=ON/OFF
  • Warns if CRYPTOPP_NO_UNALIGNED_DATA_ACCESS or CRYPTOPP_INIT_PRIORITY not defined in config.h
  • Detects and sets endianess.
  • DISABLE_ASM, DISABLE_SSSE3 and DISABLE_AESNI are not set automatically right now (using compiler detection), so they are made as CMake options.
  • Links to ws2_32 on Windows and to pthread on Linux.
  • SONAME support for Linux (version_major.version_minor)

Building tests (cryptest).

The test can be built and performes, if BUILD_TESTING is set to ON (default).
Command: make test

Building documentation using Doxygen.

I had to create a new file, Doxyfile.in, as Doxygen can't make out-of-source builds. This is the right way to do this in CMake, but we may have to maintain it 馃槙
The build yields CryptoPPRef.zip, containing all documentation, generated by Doxygen (just like GNUmakefile build).

Install.

On make install CMake installs a package, that could be used by other projects, using FindPackage(cryptopp), eliminating the need of FindCryptoPP.cmake module.
Also, make install just installs the libraries in lib, includes in include, and documentation in share/doc, honoring FHS on Linux.

Also, I've added a working .travis.yml file for Travis-CI, if you ever want to use it.

@GamePad64 GamePad64 force-pushed the master branch 2 times, most recently from 1f28c94 to b91cb64 Compare December 8, 2015 18:30
@noloader
Copy link
Collaborator

noloader commented Dec 8, 2015

This is a very good start. Thank you very much.


CMakeLists.txt, Lines 19-31, can go away. Future versions of the library will apply CRYPTOPP_NO_UNALIGNED_DATA_ACCESS and CRYPTOPP_INIT_PRIORITY. "Future versions" mean the next release (which may be 5.7 or 6.0, depending on what changes).

Folks who want unaligned data access will have to specifically set CRYPTOPP_ALLOW_UNALIGNED_DATA_ACCESS. That means confg.h block around line 500 will be deleted. We will no longer set it automatically.


I'm not sure about CMakeLists.txt, Lines 120-125. This is due to my ignorance.

+if(WIN32)
+   target_link_libraries(cryptopp-static ws2_32)
+   target_link_libraries(cryptopp-shared ws2_32)
+endif()

So we are clear... I believe the intention in the GNUmakefile is Unix-on-Windows tools like Cygwin and MinGW. MinGW requires the Winsock library ws2_32, while Cygwin does not. I don't believe its a cross-compile case, like using GCC to produce PE executables from Ubuntu under WINE (or something else exotic).

This is something you may want to test in a VM to be sure.


I'm not sure about CMakeLists.txt, Lines 125-130.

+find_package(Threads)
+target_link_libraries(cryptopp-static ${CMAKE_THREAD_LIBS_INIT})
+target_link_libraries(cryptopp-shared ${CMAKE_THREAD_LIBS_INIT})

On OS X, there are no thread libraries used. See Crypto++ build on OS X 10.8.


You might consider dropping Doxygen support. 5.6.2 and earlier did not have the make-based support. I believe Wei was generating it by hand in a one-off fashion.

We added the make-based stuff at 5.6.3 to ensure consistency and reproducible results. It was a QA thing.

If I did not need to generate the docs, then I'd avoid it altogether. (I think you have this choice).


travis.yaml concerns me.

+ sources: ['ubuntu-toolchain-r-test', 'george-edison55-precise-backports']

Effectively, we are introducing external dependencies. In addition, we are asking folks to trust someone named George Edison 55.

Can they be removed?


I'm not sure about this in travis.yaml:

+ - CXX_COMPILER=g++-5 STDLIB=libc++

The library supports a wide array or compilers, including MSVC, GCC, Intel ICC, Sun Studio, etc. I'm not sure limiting to GCC 5+ is a good idea.

In fact, I've seen a lot of problems with the experimental compilers like 5.2 and 5.3. I know they have inlining and devirtualization bugs, and I have spent a couple hundred hours trying to track them down on platforms like Debian/Aarch64.

libc++ is LLVM's C++ runtime. Is there a reason it is specified, and not, say, GNU's libstdc++ or STLport or Dinkumware?

My apologies if I am parsing things incorrectly.


Doxygen.in looks like its a copy of Doxygen. Does a pre-build step of cp Doxygen Doxygen.in work to meet the goal?

Copying Doxygen has the benefit of no Doxygen.in maintenance because we will tend to it. Doxygen's configuration file will always be maintained because we use it to build the docs.


I think that's enough for round one.

I'll get it into the testing loop, and then the real fun will begin. (Testing sucks at times. For every half hour I spend producing something, I spend hours testing it).

@noloader noloader added this to the 5.7 milestone Dec 8, 2015
@noloader
Copy link
Collaborator

noloader commented Dec 8, 2015

This is now up for debate at CmakeList.txt Pull Request.

@GamePad64
Copy link
Contributor Author

CMakeLists.txt, Lines 19-31, can go away.

Fair point, done

I'm not sure about CMakeLists.txt, Lines 120-125.

WIN32 is not defined in Cygwin using CMake version higher then (2.8.4). CMakeLists.txt requires 3.2 explicitly.

I'm not sure about CMakeLists.txt, Lines 125-130.

CMAKE_THREAD_LIBS_INIT is defined only when needed (on Linux, specifically). On OS X it is no-op.

travis.yaml concerns me.

Well, using Travis is not mandatory. It will not be able to cover all the testing needs, like running Windows builds using MSVC. Maybe, it will be better to remove Travis builds it at this point.

If I did not need to generate the docs, then I'd avoid it altogether.

Yes, me too, but I try to adapt building the library for Debian+Ubuntu, and libraries are required to have -doc package, containing full documentation. Now, I nearly have working Launchpad PPA build, that compiles the shared library, development package (static+headers), documentation, and debug symbols.

Now, if you want to generate dogumentation, you must explicitly set BUILD_DOCUMENTATION variable to ON, because it is disabled by default.

Doxygen.in looks like its a copy of Doxygen.

It has some substitution characters in file paths (between "@" characters)

Copying Doxygen has the benefit of no Doxygen.in maintenance because we will tend to it.

Tried to fix this thing in current version of commit. Now the documentation is built in-source, and then copied to build directory, eliminating the need of Doxyfile.in

@GamePad64
Copy link
Contributor Author

Rebased to include new commits.

@GamePad64
Copy link
Contributor Author

Rebased again. People are not enthusiastic about CMake here =/

noloader added a commit that referenced this pull request Dec 25, 2015
@noloader noloader merged commit a2c1e49 into weidai11:master Dec 25, 2015
@noloader
Copy link
Collaborator

@alexander - I was able to test this on a machine with Cmake. Is this expected? Shouldn't Cmake "just work" like Make and Gmake?

_OS X running 3.4.1_:

$ cd cryptopp
$ cmake
Usage

  cmake [options] <path-to-source>
  cmake [options] <path-to-existing-build>

Specify a source directory to (re-)generate a build system for it in the
current working directory.  Specify an existing build directory to
re-generate its build system.

Run 'cmake --help' for more information.

_Ubuntu 14 running 2.8.12_:

$ cd cryptopp
$ cmake
cmake version 2.8.12.2
Usage

  cmake [options] <path-to-source>
  cmake [options] <path-to-existing-build>
  ...

According to Running Cmake:

To build with just cmake change directory into where you want the binaries to be placed. For an in-place build you then run cmake and it will produce a CMakeCache.txt file that contains build options that you can adjust using any text editor...

Unfortunately, it did not build and it did not produce the artifact:

$ ls  CMakeCache.txt
ls: CMakeCache.txt: No such file or directory

We really need you guys to own this. There are folks who don't like Cmake, and they will be ping'ing me offline to remove it if it does not work.

@GamePad64
Copy link
Contributor Author

CMake requires entering source directory, so use cmake . (notice the dot). That means, that cmake will search for CMakeLists.txt in current directory.

@noloader
Copy link
Collaborator

@GamePad64 - At commit 79882d4 I added Cmake artifacts to the GNUMakefile's _distclean_ recipe. If any of the artifacts should be under _clean_ recipe, then please let me know or work-up a pull request.

@noloader noloader added the cmake label Feb 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants