Modernize compiler and linker flags

[Rossmaxx]

fixes: #4430

[Rossmaxx]

Asking @PhysSong and @DomClark to help me resolve the build fails

[DomClark]

There are a couple of things going on here, so I'll explain them bit by bit in the hope that it'll help you (and maybe others reading this too) to figure things out more easily in the future.

First, it's worth understanding how quotes work in CMake. The only data type in CMake is the string - a list is merely a string with semicolons separating the different elements. When you pass a variable as an argument to a command, it is passed as a list by default. This means that it will be split on semicolons, and each part will be passed as a separate argument. To pass a variable as a string, it can be wrapped in double quotes, which ensures that it remains a single argument. For example:

set(VARIABLE "a;b")

# These are equivalent to one another:
some_function(${VARIABLE})
some_function("a" "b")

# As are these
some_function("${VARIABLE}")
some_function("a;b")

Spaces are not list separators:

set(VARIABLE "a b")

# All three of these are equivalent
some_function(${VARIABLE})
some_function("${VARIABLE}")
some_function("a b")

Spaces do, however, separate arguments. Quotes in CMake are optional, but will be necessary if you want to include a space within a string:

# These two are equivalent, passing two arguments
some_function(a b)
some_function("a" "b")

# This is different, passing a single argument
some_function("a b")

The set command will combine all its arguments into a list (i.e., concatenate them with semicolons between), for example:

# These are equivalent
set(VARIABLE a b)
set(VARIABLE "a;b")

All in all, a good rule of thumb is to use quotes for strings, and to omit them for lists.

Now, CMake has a variety of ways to add libraries, definitions, compiler options, linker options, include directories, etc. There are typically two commands for each category: one affects all future targets defined in the current directory and its subdirectories, and the other begins with target_ and only affects the specified target. The target-specific one is generally preferred, unless you really need to modify all targets in a directory.

The values in use for each category are stored in properties on targets. Targets typically have two properties for each category: one stores the values used for building the current target itself, and the other begins with INTERFACE_ and is used for building targets that depend on the current target. The target-specific command for each category typically accepts a PRIVATE, PUBLIC, or INTERFACE argument: this specifies which properties the command will append to. PRIVATE updates only the property for building the target, INTERFACE updates only the property for building dependent targets, and PUBLIC updates both. Private options are useful when a feature is only used in source files, and public options are useful when a feature is used in header files. Interface options are less common; they're typically used for targets representing already-built third-party libraries, where the target doesn't need to be built and so only the interface values are relevant. The target properties can be modified directly using the set_property or set_target_properties commands, but the dedicated commands tend to be easier to read. When modifying properties directly, set_property has the advantage that you can append to a property rather than simply overwriting it, but set_target_properties allows you to set multiple properties at once.

Here is a summary of the categories that are most likely to be relevant to this task:

• Libraries can be added using the link_libraries and target_link_libraries commands. The target properties populated by these are LINK_LIBRARIES and INTERFACE_LINK_LIBRARIES.
• Macro definitions can be added using the add_definitions and target_compile_definitions commands. (Since CMake 3.12, add_compile_definitions is preferred over add_definitions, but we can't use that version yet.) The target properties are COMPILE_DEFINITIONS and INTERFACE_COMPILE_DEFINITIONS.
• Compiler options can be added using the add_definitions and target_compile_options commands. (Since CMake 3.12, add_compile_options is preferred over add_definitions, to separate compiler options from macro definitions.) The target properties are COMPILE_OPTIONS and INTERFACE_COMPILE_OPTIONS.
• Linker options only have dedicated commands and target properties since CMake 3.13: the commands are add_link_options and target_link_options, and the properties are LINK_OPTIONS and INTERFACE_LINK_OPTIONS. For now, the library commands and the LINK_FLAGS property have to be used instead. (There doesn't seem to be an interface version of LINK_FLAGS, so I'm not sure what to do in that case. Hopefully we don't need it.)

There is also a COMPILE_FLAGS property, which is superseded by COMPILE_OPTIONS in the same way that, since CMake 3.13, LINK_FLAGS is superseded by LINK_OPTIONS. COMPILE_FLAGS is deprecated and should not be used.

An important difference between the *_FLAGS and *_OPTIONS properties is that the newer *_OPTIONS properties are proper lists - i.e. the flags are separated by semicolons - whereas the older *_FLAGS properties separate the flags with spaces. Thus, when migrating to the newer forms, you need to make sure to change the separators appropriately. This is best done simply by passing each flag as an individual argument to the appropriate command, and letting CMake handle the separators for you.

I realise this is a bit of a wall of text, but I hope it's more generally useful than simply telling you what to do for each line. I recommend going through each change here and asking yourself: "which command is appropriate?", "should the flags be public, private, or interface?", and "how should I separate the flags?", using the information above to help you. Let me know if there's a line you're stuck on, and I can walk you through what my decision process would be.

(Finally, I'm not sure LINK_FLAGS is worth replacing with target_link_libraries. Once CMake 3.13 is available to us (which I hope would be soon - all we need to do is bump the Ubuntu version for the MinGW containers), we ought to switch to target_link_options instead, so any changes now may not last long. Secondly, I think the code is easier to read when adding flags to a property ending in "FLAGS", rather than through a command ending in "libraries".)

[Rossmaxx]

Thanks for the info dom, though to be honest, I couldn't chew most of it.

So from what you are saying, it seems like I'm handling the lists and "s wrong. I will attempt another fix but I'll wait for cmake to get bumped to 3.13, atleast that's what i understand.

[tresf]

Somewhat related; @messmerd is working on bumping the cmake version we have in the containers. The snap store has a much newer version as well (e.g. available to older Ubuntu versions) , so we can recommend that in our wiki to end-users.

[tresf]

Switching this to a draft, because @DomClark requests we use newer language features prior to merging, quoting:

Here is a summary of the categories that are most likely to be relevant to this task:
...

@Rossmaxx I would recommend that you bump the require CMake version and update this PR accordingly.

Thanks for the info dom, though to be honest, I couldn't chew most of it.

It's paramount to the project that you can "chew" it because it modernizes our CMake calls. Furthemore, the difference between using lists (versus strings with spaces) should make code cleaner and easier to maintain. If you don't want to do this task, someone else eventually will, but I'm moving this to draft until the PR's OP has a clearer direction.

[Rossmaxx]

Ahh, I found out what the actual mistake was in my PR. I used target_link_libraries instead of target_link_options wherever I was supposed to be replacing.

[Rossmaxx]

I am resetting this PR and will push the new changes soon.

[Rossmaxx]

Looking back at dom's text wall, I realize that that is exactly what he said. I will do the changes anyway and get it to compile everywhere else and then take another look when we get cmake > 3.13 on mingw

[Rossmaxx]

I'll look at the new errors later.

[Rossmaxx]

All that's left now is bumping the cmake version. Waiting for #7259 for that

[DomClark]

Removing ${LINK_FLAGS} is fine, since we never seem to set it, but why remove -shared and -Wl,-no-undefined?

[Rossmaxx]

The entire if condition looks sus, from my logic, only linux seems to use that flag properly but in the next line, for linux, everything seems to get reset. I will add it back in tho.

[DomClark]

That's a good point - I'll see if I can figure out why it was done like this.

[Rossmaxx]

Update?

Wrong button - only meant to add comments.