Skip to content

Fix issue where MACOSX_VERSION_MIN_FLAG was not set on subsequent runs of CMake in compiler-rt #87580

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

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

cjappl
Copy link
Contributor

@cjappl cjappl commented Apr 3, 2024

As discussed here:

#74394 (comment)

An unintentional change of behavior was introduced in #74394

This code introduced in #74394 :

The first time through

  • SANITIZER_MIN_OSX_VERSION is not set
  • parse -mmacosx-version-min and set MACOSX_VERSION_MIN_FLAG
  • Set and cache SANITIZER_MIN_OSX_VERSION

Subsequent times through:

  • SANITIZER_MIN_OSX_VERSION is cached
  • (BUG!!) you don't parse -mmacosx-version-min, and don't set MACOSX_VERSION_MIN_FLAG

MACOSX_VERSION_MIN_FLAG is used later in the file on this line:

if(NOT MACOSX_VERSION_MIN_FLAG)

Hoisting this assignment outside the if block returns us to the previous behavior before this commit, while maintaining the flexibility introduced with the cache variable

Copy link

github-actions bot commented Apr 3, 2024

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.
See LLVM Discourse for more information.

@cjappl
Copy link
Contributor Author

cjappl commented Apr 3, 2024

CC for a review:

@petrhosek @yln @vitalybuka

Copy link
Collaborator

@yln yln left a comment

Choose a reason for hiding this comment

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

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants