secp256k1
secp256k1 copied to clipboard
cmake: Rework flags summary
This was requested in https://github.com/hebasto/bitcoin/issues/221. The implementation follows the same approach as in https://github.com/hebasto/bitcoin/pull/93.
Here are a few excerpts from the summaries:
- Linux:
Cross compiling ....................... FALSE
Valgrind .............................. ON
Preprocessor defined macros ........... ENABLE_MODULE_ELLSWIFT=1 ENABLE_MODULE_SCHNORRSIG=1 ENABLE_MODULE_EXTRAKEYS=1 ENABLE_MODULE_ECDH=1 ECMULT_WINDOW_SIZE=15 COMB_BLOCKS=11 COMB_TEETH=6 USE_ASM_X86_64=1 VALGRIND
C compiler ............................ GNU 13.2.0, /usr/bin/cc
CMAKE_BUILD_TYPE ...................... RelWithDebInfo
C compiler flags ...................... -O2 -g -std=c90 -fPIC -fvisibility=hidden -pedantic -Wall -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef
Linker flags .......................... -fPIC -O2 -g -Wl,-soname,libsecp256k1.so.2
NOTE: The summary above may not exactly match the final applied build flags
if any additional CMAKE_* or environment variables have been modified.
To see the exact flags applied, build with the --verbose option.
- Windows:
Cross compiling ....................... FALSE
Valgrind .............................. OFF
Preprocessor defined macros ........... ENABLE_MODULE_ELLSWIFT=1 ENABLE_MODULE_SCHNORRSIG=1 ENABLE_MODULE_EXTRAKEYS=1 ENABLE_MODULE_ECDH=1 ECMULT_WINDOW_SIZE=15 COMB_BLOCKS=11 COMB_TEETH=6 _CRT_SECURE_NO_WARNINGS
C compiler ............................ MSVC 19.40.33811.0, C:/Program Files/Microsoft Visual Studio/2022/Community/VC/Tools/MSVC/14.40.33807/bin/Hostx64/x64/cl.exe
Available build configurations ........ RelWithDebInfo, Release, Debug, MinSizeRel, Coverage
Default build configuration ........... Debug
'RelWithDebInfo' build configuration:
C compiler flags .................... /DWIN32 /D_WINDOWS /Zi /O2 /Ob1 /W3 /wd4146 /wd4244 /wd4267
Linker flags ........................ /machine:x64 /debug /INCREMENTAL
'Release' build configuration:
C compiler flags .................... /DWIN32 /D_WINDOWS /O2 /Ob2 /W3 /wd4146 /wd4244 /wd4267
Linker flags ........................ /machine:x64 /INCREMENTAL:NO
'Debug' build configuration:
C compiler flags .................... /DWIN32 /D_WINDOWS /Zi /Ob0 /Od /RTC1 /W3 /wd4146 /wd4244 /wd4267
Linker flags ........................ /machine:x64 /debug /INCREMENTAL
'MinSizeRel' build configuration:
C compiler flags .................... /DWIN32 /D_WINDOWS /O1 /Ob1 /W3 /wd4146 /wd4244 /wd4267
Linker flags ........................ /machine:x64 /INCREMENTAL:NO
'Coverage' build configuration:
C compiler flags .................... /DWIN32 /D_WINDOWS /Zi /O2 /Ob1 -O0 -DCOVERAGE=1 --coverage /W3 /wd4146 /wd4244 /wd4267
Linker flags ........................ /machine:x64 /debug /INCREMENTAL --coverage
NOTE: The summary above may not exactly match the final applied build flags
if any additional CMAKE_* or environment variables have been modified.
To see the exact flags applied, build with the --verbose option.
Looks like this is at least missing link options like -compatibility_version 5.0.0 -current_version 5.1.0.
Also flags like -fPIC are shown in the C compiler flags, but not shown in the linker flags?
Good observations. These should be fixed if the rule is as in this comment:
# Print tools' flags on best-effort. Include the abstracted
# CMake flags that we touch ourselves.
Both @fanquake's comments have been addressed.
Looks like this is at least missing link options like -compatibility_version 5.0.0 -current_version 5.1.0.
The same is still missing on Linux?
All recent @fanquake's comments have been addressed.
Fixed quoting of (potentially empty) string arguments.
I'm not a fan of these flag summaries in the configure output. My main concerns are:
-
Maintenance burden:
This PR adds 90 lines of non-config logic to a section that should remain declarative and free of additional logic. -
Worsened user experience:
Inccmake, the summary appears as a warning that must be dismissed. Instead of the usualc c gworkflow, users are now forced to pressc e c e g. -
Lack of trustworthiness:
The summary is only an approximation. Users who require the exact flags will need to use a different approach anyway.
Hence, I would vote to remove the flag summary from the configure log. If there is a need for exact flags, we should provide a proper, trustworthy solution.
Hence, I would vote to remove the flag summary from the configure log. If there is a need for exact flags, we should provide a proper, trustworthy solution.
I tend to agree. But this seems like a thing where we could just follow whatever Bitcoin Core does. Is the summary still present in Core?
Hence, I would vote to remove the flag summary from the configure log.
I tend to agree. But this seems like a thing where we could just follow whatever Bitcoin Core does. Is the summary still present in Core?
It is. And people are still concerned about its correctness.
cc @fanquake
@purpleKarrot
If there is a need for exact flags, we should provide a proper, trustworthy solution.
Hard to disagree. Could you share any ideas for a possible approach?
@purpleKarrot
If there is a need for exact flags, we should provide a proper, trustworthy solution.
Hard to disagree. Could you share any ideas for a possible approach?
I think there's not much we can do because the exact flags depend on the artifact that is built. The exact way is already there: cmake --build -v, perhaps piped to grep <artifact>. If people are not aware of this functionality, we should add it to contributor guides or simply print a message that recommends this command instead of a summary.
we should add it to contributor guides
I like that approach. We should recommend people to use the Ninja generator by default (eg by setting CMAKE_GENERATOR as an environment variable in their dotfiles) and we should hint that Ninja has some Extra tools.
It is cool that cmake can create a build directory and that it provides an abstraction of the build system with cmake --build. But this functionality is mostly useful for shell scripts. For development, invoking the build system directly from inside the build directory is easier to type and more powerful:
set -x CMAKE_GENERATOR 'Ninja' # put this in your dotfiles
mkdir build
cd build
cmake .. # assuming the source directory is the parent of the current working dir
ninja help # print all build targets
ninja secp256k1 # build a single target
ninja -t list # list all of ninja's extra tools
ninja -t commands secp256k1 # print the commands that are needed to build a target
we should add it to contributor guides
I like that approach. We should recommend people to use the
Ninjagenerator by default (eg by settingCMAKE_GENERATORas an environment variable in their dotfiles) and we should hint that Ninja has some Extra tools.
I set Ninja as my default CMake generator long time ago. And I like Ninja Extra tools:
$ ninja -C build -t commands secp256k1
/usr/bin/cc -DCOMB_BLOCKS=43 -DCOMB_TEETH=6 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_ECDH=1 -DENABLE_MODULE_ELLSWIFT=1 -DENABLE_MODULE_EXTRAKEYS=1 -DENABLE_MODULE_MUSIG=1 -DENABLE_MODULE_SCHNORRSIG=1 -DUSE_ASM_X86_64=1 -DVALGRIND -O2 -g -std=c90 -fPIC -Wall -pedantic -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef -MD -MT src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o -MF src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o.d -o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o -c /home/hebasto/dev/secp256k1/secp256k1/src/precomputed_ecmult.c
/usr/bin/cc -DCOMB_BLOCKS=43 -DCOMB_TEETH=6 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_ECDH=1 -DENABLE_MODULE_ELLSWIFT=1 -DENABLE_MODULE_EXTRAKEYS=1 -DENABLE_MODULE_MUSIG=1 -DENABLE_MODULE_SCHNORRSIG=1 -DUSE_ASM_X86_64=1 -DVALGRIND -O2 -g -std=c90 -fPIC -Wall -pedantic -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef -MD -MT src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o -MF src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o.d -o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o -c /home/hebasto/dev/secp256k1/secp256k1/src/precomputed_ecmult_gen.c
/usr/bin/cc -DCOMB_BLOCKS=43 -DCOMB_TEETH=6 -DECMULT_WINDOW_SIZE=15 -DENABLE_MODULE_ECDH=1 -DENABLE_MODULE_ELLSWIFT=1 -DENABLE_MODULE_EXTRAKEYS=1 -DENABLE_MODULE_MUSIG=1 -DENABLE_MODULE_SCHNORRSIG=1 -DUSE_ASM_X86_64=1 -DVALGRIND -Dsecp256k1_EXPORTS -O2 -g -std=c90 -fPIC -Wall -pedantic -Wcast-align -Wcast-align=strict -Wextra -Wnested-externs -Wno-long-long -Wno-overlength-strings -Wno-unused-function -Wshadow -Wstrict-prototypes -Wundef -MD -MT src/CMakeFiles/secp256k1.dir/secp256k1.c.o -MF src/CMakeFiles/secp256k1.dir/secp256k1.c.o.d -o src/CMakeFiles/secp256k1.dir/secp256k1.c.o -c /home/hebasto/dev/secp256k1/secp256k1/src/secp256k1.c
: && /usr/bin/cc -fPIC -O2 -g -shared -Wl,--dependency-file=src/CMakeFiles/secp256k1.dir/link.d -Wl,-soname,libsecp256k1.so.6 -o lib/libsecp256k1.so.6.0.1 src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult.c.o src/CMakeFiles/secp256k1_precomputed.dir/precomputed_ecmult_gen.c.o src/CMakeFiles/secp256k1.dir/secp256k1.c.o && :
/home/hebasto/Downloads/cmake-4.1.1-linux-x86_64/bin/cmake -E cmake_symlink_library lib/libsecp256k1.so.6.0.1 lib/libsecp256k1.so.6 lib/libsecp256k1.so && :
The only thing left is to convince other developers and keep the CI and OSS-Fuzz logs readable.