This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Do these replacements to make use of D132608
Needs ReviewPublic

Authored by Ericson2314 on Sep 14 2022, 12:59 PM.

Details

Summary

D132608 defined new LLVMPROJ_BINARY_*DIR variables to match
GNUInstallDirs. Now we actually use them!

  • ${CMAKE_BINARY_DIR}/lib(${CMAKE_LIBDIR_SUFFIX})?\> -> ${LLVMPROJ_BINARY_LIBDIR}
  • ${CMAKE_BINARY_DIR}/bin\> -> ${LLVMPROJ_BINARY_BINDIR}
  • ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/lib${LLVM_LIBDIR_SUFFIX} -> ${LLVM_LIBRARY_OUTPUT_INTDIR}
  • ${CMAKE_BINARY_DIR}/${CMAKE_CFG_INTDIR}/bin -> ${LLVM_RUNTIME_OUTPUT_INTDIR}

It is somewhat odd to me that those last two vars start with LLVM_
not something else when they are based on CMAKE_BINARY_DIR and not LLVM_BINARY_DIR, but that can
be tackled some other time.

Diff Detail

Event Timeline

Ericson2314 created this revision.Sep 14 2022, 12:59 PM
Herald added a reviewer: rafauler. · View Herald Transcript
Herald added a reviewer: Amir. · View Herald Transcript
Herald added a reviewer: maksfb. · View Herald Transcript
Herald added a reviewer: NoQ. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Ericson2314 requested review of this revision.Sep 14 2022, 12:59 PM
Herald added projects: Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript

I don't see anything wrong with this change per se, but I'm conflicted on the name of the variable. These are not standard variables but are encroaching on the CMake namespace. What happens if upstream decides to use these names? I think that we should keep the names in the LLVM_ namespace. However, I realize that the variable itself is not being touched here and this is a mechanical replacement.

Rebase, rename variables

Ericson2314 edited the summary of this revision. (Show Details)Nov 7 2022, 8:04 AM
Ericson2314 edited the summary of this revision. (Show Details)