Minor improvement suggestion for the readme. I find this alternative way of using cmake a bit more comfortable because I don't like to change the directory.
It's just a suggestion based on personal preference, if this is too minor of an improvement feel free to close.
I happen to find this a tiny bit more comfortable, too, but IIRC we had discussed this (I can't find the discussion) and @hebasto argued that the tutorial cds into the build directory. I also think that mkdir build && cd build is a bit more self-documentary to people not familiar with CMake because it makes clear that build is a directory and not some CMake command or something.
No strong opinion from my side, but I think there are good reasons for what we have currently.
@real-or-random alright, thanks for the info, closing it for now
FWIW, while working on the CMake staging branch for Bitcoin Core, the rough consensus has been reached not to suggest using cd in the build docs (see https://github.com/bitcoin/bitcoin/blob/master/doc/build-*.md).
Ok, reopening since @real-or-random seemed in favor and I take from @hebasto 's comment that the argument for cd-ing seems not so relevant anymore and this would align the instructions here with bitcoin core.
87 | + $ cmake -B build 88 | + $ cmake --build build 89 | + $ ctest --test-dir build # run the test suite 90 | + $ sudo cmake --install build # optional 91 | 92 | To compile optional modules (such as Schnorr signatures), you need to run `cmake` with additional flags (such as `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON`). Run `cmake .. -LH` to see the full list of available flags.
This has to be adjusted accordingly:
To compile optional modules (such as Schnorr signatures), you need to run `cmake` with additional flags (such as `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON`). Run `cmake -B build -LH` to see the full list of available flags.
Or even:
To compile optional modules (such as Schnorr signatures), you need to run `cmake` with additional flags (such as `-DSECP256K1_ENABLE_MODULE_SCHNORRSIG=ON`). Run `cmake -B build -LH` or `ccmake -B build` to see the full list of available flags.
done
90 | @@ -92,11 +91,11 @@ To compile optional modules (such as Schnorr signatures), you need to run `cmake 91 | To alleviate issues with cross compiling, preconfigured toolchain files are available in the `cmake` directory. 92 | For example, to cross compile for Windows: 93 | 94 | - $ cmake .. -DCMAKE_TOOLCHAIN_FILE=../cmake/x86_64-w64-mingw32.toolchain.cmake 95 | + $ cmake --build build -DCMAKE_TOOLCHAIN_FILE=../cmake/x86_64-w64-mingw32.toolchain.cmake
$ cmake --build build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake
done
utACK 93363fa0743b11c7daf04db457b0dba3cec07af2
96 | 97 | To alleviate issues with cross compiling, preconfigured toolchain files are available in the `cmake` directory. 98 | For example, to cross compile for Windows: 99 | 100 | - $ cmake .. -DCMAKE_TOOLCHAIN_FILE=../cmake/x86_64-w64-mingw32.toolchain.cmake 101 | + $ cmake --build build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake
My apologies for the typo:
$ cmake -B build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake
It was my fault.
fixed
101 | + $ cmake --build build -DCMAKE_TOOLCHAIN_FILE=cmake/x86_64-w64-mingw32.toolchain.cmake 102 | 103 | To cross compile for Android with [NDK](https://developer.android.com/ndk/guides/cmake) (using NDK's toolchain file, and assuming the `ANDROID_NDK_ROOT` environment variable has been set): 104 | 105 | - $ cmake .. -DCMAKE_TOOLCHAIN_FILE="${ANDROID_NDK_ROOT}/build/cmake/android.toolchain.cmake" -DANDROID_ABI=arm64-v8a -DANDROID_PLATFORM=28 106 | + $ cmake --build build -DCMAKE_TOOLCHAIN_FILE="${ANDROID_NDK_ROOT}/build/cmake/android.toolchain.cmake" -DANDROID_ABI=arm64-v8a -DANDROID_PLATFORM=28
Same here:
$ cmake -B build -DCMAKE_TOOLCHAIN_FILE="${ANDROID_NDK_ROOT}/build/cmake/android.toolchain.cmake" -DANDROID_ABI=arm64-v8a -DANDROID_PLATFORM=28
fixed
To unify all examples, -S . can be dropped from the example for Windows.
85 | - $ ctest # run the test suite 86 | - $ sudo cmake --install . # optional 87 | + $ cmake -B build 88 | + $ cmake --build build 89 | + $ ctest --test-dir build # run the test suite 90 | + $ sudo cmake --install build # optional
If you touch this anyway, perhaps it could be made clear that build is a directory here (and it can't hurt to explain the steps in more detail).
$ cmake -B build # Generate a build system in subdirectory "build"
$ cmake --build build # Run the actual build process
$ ctest --test-dir build # Run the test suite
$ sudo cmake --install build # Install the library into the system (optional)
Sounds good, taken as suggested!
Thanks, took the suggested changes and removed the -S . from the windows example too.
LGTM but sorry, a last nit: Perhaps then the comments for the autotools build should also be improved for consistency.
$ ./autogen.sh # Generate a ./configure script
$ ./configure # Generate a build system
$ make # Run the actual build process
$ make check # Run the test suite
$ sudo make install # Install the library into the system (optional)
And indenting to the same column is more readable, and then should also be done for the CMake instructions for consistency.
@real-or-random no worries, applied all the suggested changes
ACK 2ac9f558c43b5ef11f0d6e1088a8295a6d162659.
utACK 2ac9f558c43b5ef11f0d6e1088a8295a6d162659