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

cmake: Rework flags summary #1558

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jun 28, 2024

This was requested in hebasto/bitcoin#221. The implementation follows the same approach as in hebasto/bitcoin#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.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

The note is a bit verbose, but I don't have a convincing suggestion to shorten it.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, when I wrote the previous comment, I assumed this PR is in the hebasto/bitcoin repo for cmake staging... 🤦‍♂️ :

utACK aa4c5f3 this matches the implementation in hebasto/bitcoin#93 and has been revieed there

@fanquake
Copy link
Member

fanquake commented Jul 1, 2024

Looks like this is at least missing link options like -compatibility_version 5.0.0 -current_version 5.1.0.

@fanquake
Copy link
Member

fanquake commented Jul 1, 2024

Also flags like -fPIC are shown in the C compiler flags, but not shown in the linker flags?

@real-or-random
Copy link
Contributor

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.

@hebasto hebasto force-pushed the 240628-cmake-summary branch from aa4c5f3 to 765681b Compare July 1, 2024 14:07
@hebasto
Copy link
Member Author

hebasto commented Jul 1, 2024

Both @fanquake's comments have been addressed.

@fanquake
Copy link
Member

fanquake commented Jul 1, 2024

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?

@hebasto hebasto force-pushed the 240628-cmake-summary branch 2 times, most recently from 6fbe2bc to cb574bf Compare July 1, 2024 20:19
@hebasto
Copy link
Member Author

hebasto commented Jul 1, 2024

All recent @fanquake's comments have been addressed.

@hebasto hebasto force-pushed the 240628-cmake-summary branch from cb574bf to 8ff8b25 Compare July 2, 2024 07:47
@hebasto
Copy link
Member Author

hebasto commented Jul 2, 2024

Fixed quoting of (potentially empty) string arguments.

@hebasto hebasto marked this pull request as draft September 18, 2024 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants