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

Turn on CodeQL and fix BinSkim regressions #805

Merged
merged 4 commits into from
Nov 10, 2022

Conversation

BillyONeal
Copy link
Member

No description provided.

@Neumann-A
Copy link
Contributor

How about using CMake presets instead and be a good example how CMake should be done ?

if(CMAKE_BUILD_TYPE STREQUAL "Release")
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Zi /guard:cf")
set(CMAKE_EXE_LINKER_FLAGS "${CMAKE_EXE_LINKER_FLAGS} /DEBUG /debugtype:cv,fixup /guard:cf")
# either MSVC, or clang-cl
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, we should either:

  1. add_compile_options() / add_link_options()
  2. Move these to a Toolchain file

Modifying CMAKE_C(XX)_FLAGS in the CMakeLists.txt is always a smell. That said, this works and follows what came before, so not requiring changes at this time.

@BillyONeal
Copy link
Member Author

How about using CMake presets instead and be a good example how CMake should be done ?
[...]
In general, we should either:

  1. add_compile_options() / add_link_options()
  2. Move these to a Toolchain file

Modifying CMAKE_C(XX)_FLAGS in the CMakeLists.txt is always a smell. That said, this works and follows what came before, so not requiring changes at this time.

This is a controversial opinion among the vcpkg maintainers, but I still believe that CMake expecting application authors to separate warnings manipulation into separate files is unreasonable. And I cite as evidence that I've reviewed a lot of things that are shipped into vcpkg ports and never ever seen a project do it. Separate files is a statement that separate people are interested in the values, and that isn't true for diagnostics.

Contrast with /machine: switches in similar that make sense to be part of the "toolchain".

@BillyONeal BillyONeal merged commit 5fdee72 into microsoft:main Nov 10, 2022
@BillyONeal BillyONeal deleted the codeql branch November 10, 2022 21:25
@Neumann-A
Copy link
Contributor

And I cite as evidence that I've reviewed a lot of things that are shipped into vcpkg ports and never ever seen a project do it.

Most ports predate presets and most projects don't care about proper CMake ;)
VTK/Qt in general give an option to opt out of the flag setting business.

@BillyONeal
Copy link
Member Author

VTK/Qt in general give an option to opt out of the flag setting business.

And so do we. VCPKG_DEVELOPMENT_WARNINGS defaults to off.

@Neumann-A
Copy link
Contributor

And so do we. VCPKG_DEVELOPMENT_WARNINGS defaults to off.

That is just the warning flag setting business and leaves at least:

vcpkg-tool/CMakeLists.txt

Lines 120 to 130 in 5fdee72

string(APPEND CMAKE_C_FLAGS " -FC -permissive- -utf-8 /guard:cf")
string(APPEND CMAKE_CXX_FLAGS " /EHsc -FC -permissive- -utf-8 /guard:cf")
string(APPEND CMAKE_C_FLAGS_RELEASE " /Zi")
string(APPEND CMAKE_CXX_FLAGS_RELEASE " /Zi")
string(APPEND CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO " /DEBUG /INCREMENTAL:NO /debugtype:cv,fixup /guard:cf")
string(APPEND CMAKE_EXE_LINKER_FLAGS_RELEASE " /DEBUG /INCREMENTAL:NO /debugtype:cv,fixup /guard:cf")
if (MSVC_CXX_ARCHITECTURE_ID STREQUAL "x64")
string(APPEND CMAKE_EXE_LINKER_FLAGS_RELWITHDEBINFO " /CETCOMPAT")
string(APPEND CMAKE_EXE_LINKER_FLAGS_RELEASE " /CETCOMPAT")
endif()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants