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: Remove need to fiddle with CMAKE_C_FLAGS / CMAKE_CXX_FLAGS #1409

Merged
merged 1 commit into from Apr 10, 2019

Conversation

mwtoews
Copy link
Member

@mwtoews mwtoews commented Apr 4, 2019

This PR is to help modernize the CMake setup (#1263)

There a are likely several ways to accomplish this, but this is the cleanest way that I can see. Most of the custom flags added to date are warning flags. There are now cached into PROJ_C_WARN_FLAGS/PROJ_CXX_WARN_FLAGS, then applied to the relevant targets.

Currently, these target compile options PRIVATE, since warning flags should not change the behavior of the library/utilities. But perhaps these should be INTERFACE?

Also, /D_CRT_SECURE_NO_WARNINGS is not exactly a warning flag, so it's been bumped into a global add_definitions.

The only remaining CMAKE_C_FLAGS modification is related to an Intel flag, which I may address another day.

@mwtoews
Copy link
Member Author

mwtoews commented Apr 5, 2019

Oh, nearly forgot to mention moving the interprocedural optimization flag -flto as a target compile option. It is currently set as PRIVATE, but should it be PUBLIC or INTERFACE instead? From what I understand, this option is both a compile and linker flag.

The two failures are related to implicit function declaration, which changed between C89 and C99 Standards. PR #1410 makes it bit more clear to see the compile command to help sort out this type of situation.

Remove (most) needs to fiddle with CMAKE_C_FLAGS / CMAKE_CXX_FLAGS
@mwtoews
Copy link
Member Author

mwtoews commented Apr 5, 2019

The two C89 failures are now sorted, which needed "all warnings" added to CMAKE_REQUIRED_FLAGS (I'm nearly certain there is a CMake bug with check_c_source_compiles, as the C99 example clearly fails with -std=c89; also, CMake has >2k open issues, so I'd rather not dig any deeper).

This PR is ready for review. I.e., are there any opinions on the use of PRIVATE vs. PUBLIC or INTERFACE as mentioned above for the scope of warning flags or IPO?

@kbevers
Copy link
Member

kbevers commented Apr 9, 2019

This PR is ready for review. I.e., are there any opinions on the use of PRIVATE vs. PUBLIC or INTERFACE as mentioned above for the scope of warning flags or IPO?

Overall I think this is great. I have no idea what is the right thing to do regarding the PRIVATE/PUBLIC thing. What will be different if PUBLIC is chosen instead of PRIVATE?

@mwtoews
Copy link
Member Author

mwtoews commented Apr 10, 2019

PRIVATE is the easiest option to go with, as these flags only go to those targets, and don't get imported or passed around to others.

Warning flags are generally added to targets with PRIVATE (e.g. here, here). However, I'm aware the folks in the libgeos camp (e.g. dbaston's modern-cmake) have chosen to go with INTERFACE for warnings, and generally have a different approach to applying the flags to targets (for instance, I generally find if-conditions more readable than generator expressions).

As for IPO/LTO, PRIVATE produces the same compile commands as existing, so this selection is probably fine. But in the CMake realm, I'm not sure if those properties need to be retained if imported/used by other targets, since it's a compiler and linker flag.

@kbevers
Copy link
Member

kbevers commented Apr 10, 2019

After reading this I think that PRIVATE is the right option here. It would be interesting to know why GEOS chose to use INTERFACE over PRIVATE though.

@mwtoews
Copy link
Member Author

mwtoews commented Apr 10, 2019

Yup, that makes sense. Nevertheless, the scope keyword is simple to toggle, if there is a good argument to be made.

@kbevers
Copy link
Member

kbevers commented Apr 10, 2019

Right, let's just go with what you've made so far. We'll change it if is needed at some point in the future.

@kbevers kbevers merged commit 309eab8 into OSGeo:master Apr 10, 2019
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.

None yet

2 participants