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

CMakeLists.txt: various fixes #535

Closed
wants to merge 1 commit into from

Conversation

DasRoteSkelett
Copy link

Bumped minimal version of cmake to 3.5, because versions below 3.5 will be removed soon from cmake. Removed manual setting of optimization flags. These are already set to decent defaults, and it should be possible to set them from the outside if need be.

Also made an option SOPHUS_INSANE_CFLAGS which defaults to off. The issue is, that (more) modern compilers will not compile the tests / code with -Werror on. Werror is therefore harmful to production, albeit useful for developers. Hence, it is an option to be set.

Bumped minimal version of cmake to 3.5, because versions below
3.5 will be removed soon from cmake. Removed manual setting
of optimization flags. These are already set to decent defaults, and
it should be possible to set them from the outside if need be.

Also made an option SOPHUS_INSANE_CFLAGS which defaults to off. The
issue is, that (more) modern compilers will not compile the tests / code
with -Werror on. Werror is therefore harmful to production, albeit
useful for developers. Hence, it is an option to be set.

Signed-off-by: Matthias Schoepfer <matthias.schoepfer@googlemail.com>
@strasdat
Copy link
Owner

@DasRoteSkelett Thanks for your PR!

I did merge larger PR (#538) today which bumps cmake to 3.16.

the issue is, that (more) modern compilers will not compile the tests / code with -Werror on.

Hm, I'm a fan of -Werror and rather have those issues fixed or specific problematic warnings disabled (globally or in the corresponding file using https://stackoverflow.com/a/48426846). #538 moves to more modern compiler toolchain and hopefully fixes the issues you saw.

I'm closing your PR for now, but feel free to reopen an updated PR if you still see issues.

@strasdat strasdat closed this Jun 10, 2024
@DasRoteSkelett
Copy link
Author

Hi @strasdat ,

I see your point from the side of a Developer. Please bear in mind, that there are also so called users.

Regards

@strasdat
Copy link
Owner

strasdat commented Jun 10, 2024

@DasRoteSkelett I do see your point, and I'm also reading up on the topic e.g.

https://discourse.cmake.org/t/how-do-i-replace-the-compiler-options-for-one-target/7815

My main objective is to use Werror etc in CI. I think I like to come up with a clean solution which does not set any of those CMAKE_<LANG>_FLAGS inside the CMakeLists.txt.

Possibly cmake presets are the way to go:

https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html

Thanks for pointing out that issue...

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.

2 participants