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

Use target_link_options to pass linker options #108

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

hbwang15
Copy link

some spell errors found when i use cmake-init, see the differences below

@sbusch42
Copy link
Member

Thank you for the PR. However, I would not call this a "spell error". You are replacing one command (target_link_libraries) with another command (target_link_options). This command did not exist before in cmake, it only entered the language in version 3.13 (see https://cmake.org/cmake/help/latest/command/target_link_options.html).

Now, you are right that the new command is semantically the "right" one, now that they finally decided to differentiate it from the former command. However, both commands do the same thing in the end, and if we want to use the new command, we have to raise the minimum version requirement to 3.13 from 3.0, which it is at now. We should discuss if we want to do this for a change that does not have any functional improvement.

@hbwang15: Please raise the mimimum cmake version in the main CMakeLists.txt and add that to the PR.

Everyone: What's your opinion about this change?

@sbusch42 sbusch42 requested a review from scheibel June 16, 2021 03:32
@hbwang15 hbwang15 closed this Jun 16, 2021
@hbwang15 hbwang15 deleted the feature/spellerrorfix branch June 16, 2021 03:52
@hbwang15 hbwang15 restored the feature/spellerrorfix branch June 16, 2021 03:53
@hbwang15 hbwang15 reopened this Jun 16, 2021
@hbwang15
Copy link
Author

Thank you for the PR. However, I would not call this a "spell error". You are replacing one command (target_link_libraries) with another command (target_link_options). This command did not exist before in cmake, it only entered the language in version 3.13 (see https://cmake.org/cmake/help/latest/command/target_link_options.html).

Now, you are right that the new command is semantically the "right" one, now that they finally decided to differentiate it from the former command. However, both commands do the same thing in the end, and if we want to use the new command, we have to raise the minimum version requirement to 3.13 from 3.0, which it is at now. We should discuss if we want to do this for a change that does not have any functional improvement.

@hbwang15: Please raise the mimimum cmake version in the main CMakeLists.txt and add that to the PR.

Everyone: What's your opinion about this change?

Sorry for my limit knowledge on cmake , I just misunderstand about the command {target_link_libraries} as use for adding link options, As cmake already give more clear command, that is {target_link_options}, so i strongy suggest to upgrade cmake-init to make it more clear for users.

@hbwang15 hbwang15 changed the title spell error fix Use target_link_options to pass linker options Jun 17, 2021
@scheibel
Copy link
Member

scheibel commented Jul 2, 2021

I'm, too, thinking about our minimum required CMake versions from time to time and for me it depends on the available CMake versions for our target platforms, the availability of fallbacks for modern features/interfaces, and the severity if missing features if a platform does not yet support a more current version.
A quick check for the latest Ubuntu LTS (Ubuntu 20.04) shows availability of CMake 3.16.

Following my checklist we would have:

  • Availability of feature on (one/my) target platform
  • Availability of a fallback, that is the current implementation
  • No missing feature as a fallback is available

However, I'm still undecided and suggest to investigate / clarify the following issues:

  • Is CMake 3.16 available on all our target platforms?
  • What is the oldest release we want to support? (currently we use features up to CMake 3.2 with graceful degration to 3.0)
  • How important is it for us to use the most specific interface for the same task, even if it limits portability of our template?

Summarizing, I'm glad that this interface exists in modern CMake but I'm not sure how it should influence this project as our template works completely fine with CMake 3.0.

@scheibel
Copy link
Member

scheibel commented Jul 2, 2021

Can we provide an own implementation of target_link_options that is implemented in cmake/Custom.cmake if the official version is not available?

@scheibel
Copy link
Member

Somehow, this seems to be an implementation to #103.

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

3 participants