Skip to content

Improve detection of macOS deployment target, for choosing pmr implementation.#5027

Merged
KiterLuc merged 1 commit intodevfrom
teo/pmr-fix-2
May 30, 2024
Merged

Improve detection of macOS deployment target, for choosing pmr implementation.#5027
KiterLuc merged 1 commit intodevfrom
teo/pmr-fix-2

Conversation

@teo-tsirpanis
Copy link
Copy Markdown
Member

SC-48427

#4995 changed detection of std::pmr from trying to run a C++ file referencing pmr APIs, to trying to compile it, which works in cross compilation scenarios and fixed Conda failures for reasons I do not understand.

However in macOS versions prior to 14 the pmr headers exist, but binaries compiled with it fail ot run. There was already a check to force using the vendored pmr in such versions but it was defective, because it checked the MACOS_DEPLOYMENT_TARGET environment variable, which might not exist. Before #4995, the runtime failure on these versions would be caught by try_run. This PR updates the detection script to use the CMAKE_OSX_DEPLOYMENT_TARGET variable, which gets automatically filled by CMake either from the aforementioned environment variable or automatically.


TYPE: NO_HISTORY

@teo-tsirpanis teo-tsirpanis requested a review from KiterLuc May 30, 2024 11:26
message(STATUS "Using vendored cpp17::pmr for polymorphic allocators")
return()
endif()
if (APPLE AND CMAKE_OSX_DEPLOYMENT_TARGET LESS 14)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Should LESS be VERSION_LESS? 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think so, that is for multi-component (dot separated) comparisons AFAICT

@KiterLuc KiterLuc requested review from davisp and ihnorton May 30, 2024 11:35
message(STATUS "Using vendored cpp17::pmr for polymorphic allocators")
return()
endif()
if (APPLE AND CMAKE_OSX_DEPLOYMENT_TARGET LESS 14)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think so, that is for multi-component (dot separated) comparisons AFAICT

@KiterLuc KiterLuc merged commit 505b39a into dev May 30, 2024
@KiterLuc KiterLuc deleted the teo/pmr-fix-2 branch May 30, 2024 13:45
Copy link
Copy Markdown
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

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.

4 participants