This is an archive of the discontinued LLVM Phabricator instance.

Completely avoid building Apple simulator on non-Darwin platforms.
ClosedPublic

Authored by chaoren on Nov 5 2015, 12:19 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

chaoren updated this revision to Diff 39394.Nov 5 2015, 12:19 PM
chaoren retitled this revision from to Completely avoid building Apple simulator on non-Darwin platforms..
chaoren updated this object.
chaoren added a subscriber: lldb-commits.
zturner edited edge metadata.Nov 5 2015, 12:21 PM

Isn't this going to have the same problem as before? If CMake finds a source file in the directory that is not added to one of the targets, it will give you an error. With this patch, on Linux or Windows PlatformAppleSimulator.cpp will be on disk, but not part of the lldbPluginPlatformMacOSX target, and as a result CMake will complain.

LLVM_OPTIONAL_SOURCES should avoid that.

It's not actually CMake complaining but the LLVM cmake scripts.

Ahh makes sense, I didn't know about that. I think a cleaner way to do this is to just add everything to PLUGIN_PLATFORM_MACOSX_SOURCES in the very first list(APPEND). Then just do this:

if (NOT CMAKE_SYSTEM_NAME MATCHES "Darwin")
    set(LLVM_OPTIONAL_SOURCES
        PlatformAppleSimulator.cpp
        PlatformiOSSimulator.cpp
    )
    list(REMOVE_ITEM PLUGIN_PLATFORM_MACOSX_SOURCES ${LLVM_OPTIONAL_SOURCES})
endif()

I haven't seen anything else that uses LLVM_OPTIONAL_SOURCES, but it's conceivable that there might be items in the list already, and I would like to avoid doing anything to it besides just appending.

zturner accepted this revision.Nov 5 2015, 12:39 PM
zturner edited edge metadata.

I find the code to be a fair bit harder to understand as a result, but I guess it's not the end of the world.

Searching the source of LLVM's CMake infrastructure, this variable exists solely to make LLVM not complain about missing files in the current directory, so I can't see any way that anything outside of the current directory's CMakeList.txt could have modified it intentionally. So I think it's safe, in the same way that the pattern for setting library dependencies is to write something like this:

set(LLVM_LINK_COMPONENTS
  Analysis
  AsmParser
  Core
  Support
  )

rather than appending. Anyway, up to you.

This revision is now accepted and ready to land.Nov 5 2015, 12:39 PM
This revision was automatically updated to reflect the committed changes.