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/compilerFlags.cmake: properly detect availability of flags #1252

Merged
merged 1 commit into from
Aug 10, 2020

Conversation

tpetazzoni
Copy link
Contributor

Instead of relying on fragile and complex logic to decide if a
compiler flag is available or not, use the check_c_compiler_flag()
macro provided by the CMake standard library.

This for example avoids using -fcf-protection on architectures that
don't support this option.

Signed-off-by: Thomas Petazzoni [email protected]

Instead of relying on fragile and complex logic to decide if a
compiler flag is available or not, use the check_c_compiler_flag()
macro provided by the CMake standard library.

This for example avoids using -fcf-protection on architectures that
don't support this option.

Signed-off-by: Thomas Petazzoni <[email protected]>
@codecov
Copy link

codecov bot commented Jul 19, 2020

Codecov Report

Merging #1252 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1252   +/-   ##
=======================================
  Coverage   71.91%   71.91%           
=======================================
  Files         152      152           
  Lines       17674    17674           
=======================================
  Hits        12710    12710           
  Misses       4964     4964           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea63cc...4a76388. Read the comment docs.

@tbeu tbeu added the CMake Configuration issues related with CMake label Aug 10, 2020
Copy link
Member

@tbeu tbeu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@tbeu tbeu merged commit dd2d181 into Exiv2:master Aug 10, 2020
@clanmills
Copy link
Collaborator

It is possible to do a similar change in 0.27-maintenance. By co-incidence @ryandesign asked for this to be done to support MacPorts. I was unsuccessful when I tried to do this with #1245. My dyslexia got the better of me. I find this kind of change very difficult to get right. As you can see, Ryan gave me a detailed review, however I thought I'd probably make matters worse when I try to fix my mistakes.

However, I did set up a Yosemite VM with Xcode 6.1. So, I'm happy to test anybody's PR to address this.

@tbeu tbeu added this to the v0.28 milestone Aug 10, 2020
This was referenced Aug 10, 2020
jtojnar pushed a commit to jtojnar/exiv2 that referenced this pull request Sep 4, 2020
…xiv2#1252)

Instead of relying on fragile and complex logic to decide if a
compiler flag is available or not, use the check_c_compiler_flag()
macro provided by the CMake standard library.

This for example avoids using -fcf-protection on architectures that
don't support this option.

Signed-off-by: Thomas Petazzoni <[email protected]>

(cherry picked from commit dd2d181)

When resolving the conflict from applying the patch, I also took the liberty
of re-indenting the snippet correcly and fixing mismatching
HAS_FCF_PROTECTION and HAS_FSTACK_PROTECTOR_STRONG variables
(the conditionals used GCC_ prefix but the variables were definded without it).

Signed-off-by: Jan Tojnar <[email protected]>
@jtojnar
Copy link
Contributor

jtojnar commented Sep 4, 2020

I cherry picked this over to 0.27 branch: #1271

clanmills added a commit that referenced this pull request Sep 4, 2020
Properly detect availability of flags in cmake/compilerFlags.cmake (#1252)
@clanmills clanmills mentioned this pull request Jan 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CMake Configuration issues related with CMake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants