This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Move the AIX archiver settings to a module
ClosedPublic

Authored by phosek on Dec 7 2021, 12:17 PM.

Details

Summary

This allows their reuse across projects. The name of the module
is intentionally generic because we would like to move more platform
checks there.

Diff Detail

Event Timeline

phosek created this revision.Dec 7 2021, 12:17 PM
phosek requested review of this revision.Dec 7 2021, 12:17 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2021, 12:17 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

I'm thinking about moving https://github.com/llvm/llvm-project/blob/cfe5d768be95ae0a62cf430e56e7762cebf81a65/llvm/cmake/modules/HandleLLVMOptions.cmake#L174 and https://github.com/llvm/llvm-project/blob/cfe5d768be95ae0a62cf430e56e7762cebf81a65/compiler-rt/cmake/Modules/UseLibtool.cmake into CheckPlatform.cmake since this is logic we want to reuse across projects. Let me know if you have a better suggestion for the name of this module.

Let me know if you have a better suggestion for the name of this module.

SetArchiverForPlatform? Or something that conveys the fact that (1) this is basically about ar, and (2) it actually performs an action (it sets global variables).

Let me know if you have a better suggestion for the name of this module.

SetArchiverForPlatform? Or something that conveys the fact that (1) this is basically about ar, and (2) it actually performs an action (it sets global variables).

SetPlatformToolchainTools?

kaz7 added a comment.Dec 7 2021, 4:07 PM

+1 for SetPlatformToolchainTools. :-)

lkail added a subscriber: lkail.Dec 7 2021, 6:04 PM
simoll added a subscriber: simoll.Dec 9 2021, 4:45 AM
kaz7 added a comment.Dec 13 2021, 10:21 PM

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

For the record, the review guidelines we have for libc++/libc++abi do not necessarily apply to compiler-rt, so there's no requirement that someone from the libc++ review group gives an approval for this to make progress.

kaz7 added a comment.Dec 14 2021, 7:17 AM

Is it OK to accept this patch by me? Or should we wait for @ldionne? Just curious.

For the record, the review guidelines we have for libc++/libc++abi do not necessarily apply to compiler-rt, so there's no requirement that someone from the libc++ review group gives an approval for this to make progress.

Thank you for clarifying.

phosek updated this revision to Diff 397645.Jan 5 2022, 11:18 AM

Renamed the module to SetPlatformToolchainTools.cmake as suggested.

smeenai accepted this revision.Jan 5 2022, 11:35 AM

LGTM

cmake/Modules/SetPlatformToolchainTools.cmake
3 ↗(On Diff #397645)

Super nit: using an unquoted ${} inside an if is a bad idea cos of https://cmake.org/cmake/help/latest/command/if.html#variable-expansion. Should be either if(CMAKE_SYSTEM_NAME or if("${CMAKE_SYSTEM_NAME}"

This revision is now accepted and ready to land.Jan 5 2022, 11:35 AM
smeenai added inline comments.Jan 5 2022, 11:36 AM
cmake/Modules/SetPlatformToolchainTools.cmake
3 ↗(On Diff #397645)

(I know it's just a copypasta of the existing code, but might as well fix it while we're here)

phosek updated this revision to Diff 397652.Jan 5 2022, 11:46 AM
phosek marked 2 inline comments as done.
This revision was landed with ongoing or failed builds.Jan 5 2022, 11:46 AM
This revision was automatically updated to reflect the committed changes.