build: remove custom variables for cmark handling

[compnerd]

Use the imported target to handle the include path and link libraries.
This allows CMake to properly handle dependencies and removes the
custom handling logic that we have relied on now that CMark properly
supports export targets.

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

[compnerd]

@swift-ci please clean test

[compnerd]

CC: @Rostepher @edymtt @xiaobai @davezarzycki @drodriguez

[davezarzycki]

Hi @compnerd -- Thanks for the CC. I've verified that this does not break my unified build setup.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 8b351de56d62e56c0335acacf2a01a9e6837f5d0

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 8b351de56d62e56c0335acacf2a01a9e6837f5d0

[compnerd]

@swift-ci please clean test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 8b351de56d62e56c0335acacf2a01a9e6837f5d0

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 8b351de56d62e56c0335acacf2a01a9e6837f5d0

[drodriguez]

Left a comment in commonmark/cmark#327 of why I think it is not working as intended.

[compnerd]

Ugh, yeah, that would work too. I ended up with just extracting the variable for the export file and then substituting that in. But you’re spot on with the analysis.

[compnerd]

Please test with following PRs:
apple/swift-cmark#19

@swift-ci please clean test

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 507892d7bb3ea34d5b12da2ab79b937c03e821c1

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 507892d7bb3ea34d5b12da2ab79b937c03e821c1

[compnerd]

@swift-ci please clean test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 10f45582f674fe11e901808979dcf9f9f368af0b

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 10f45582f674fe11e901808979dcf9f9f368af0b

[compnerd]

@swift-ci please clean test

[drodriguez]

@swift-ci please test Windows platform

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 29d9cccd4875ed39e2364a4094257616be1433ab

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 29d9cccd4875ed39e2364a4094257616be1433ab

[compnerd]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - f53ae41

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - f53ae41

[MaxDesiatov]

@compnerd I've checked out this branch locally, but it fails with this build error:

swift/lib/Markup/Markup.cpp:18:10: fatal error: 'cmark.h' file not found
#include "cmark.h"

It seems like the change from

  include_directories(
      "${CLANG_BUILD_INCLUDE_DIR}"
      "${CLANG_MAIN_INCLUDE_DIR}"
      "${CMARK_MAIN_INCLUDE_DIR}"
      "${CMARK_BUILD_INCLUDE_DIR}")

to

  include_directories(${CLANG_BUILD_INCLUDE_DIR}
    ${CLANG_MAIN_INCLUDE_DIR})

in this branch breaks that, as no replacement include directory is specified for the swiftMarkup target.

I assumed that something like

target_include_directories(swiftMarkup PRIVATE 
  ${cmark_INCLUDE_DIR})

in lib/Markup/CMakeLists.txt would fix this, but it doesn't. I've also checked generated cmarkConfig.cmake and cmarkTargets.cmake, none of them seem to define anything related to include paths. Is this something that should be fixed in cmark CMake files to declare a PUBLIC header, or am I missing something?

[compnerd]

I think that we may need to sync with upstream again?

[MaxDesiatov]

Nothing has changed upstream in their CMake files though, at least when comparing their master to Apple's fork master: apple/swift-cmark@master...commonmark:master

[MaxDesiatov]

I'm seeing this both upstream and in the swift-cmark fork:

  install(FILES
    cmark.h
    ${CMAKE_CURRENT_BINARY_DIR}/cmark_export.h
    ${CMAKE_CURRENT_BINARY_DIR}/cmark_version.h
    DESTINATION include
    )

cmark.h is not in build/Ninja-ReleaseAssert/cmark-macosx-x86_64 (which I assume is what CMAKE_CURRENT_BINARY_DIR is) by the time swiftMarkup target is built, so I guess the install step for cmark hasn't happened at that time, but is required for the swiftMarkup target to get access to the header?

Sorry for such a trivial question, I'm just trying to wrap my head around how these dependencies are built, how their include paths are set up, and how target dependencies are structured in the toolchain CMake files. So I decided to start with something small such as cmark.

[MaxDesiatov]

@compnerd another thing I wanted to try was to migrate the cmark dependency from build-script-impl to build-script or maybe even a direct CMake dependency, that's how I stumbled upon this PR. Do you think this migration would be worth trying as an exercise to get more comfortable with how CMake is used in the project? If so, should it be migrated to be built from Python build-script or should it directly become a CMake dependency pulled with something like ExternalProject_Add or add_subdirectory?

[compnerd]

The current CMake usage is not something we should be aiming to emulate.

What you’re thinking about (using cmake properly) is a good thing. Making it built in build-script and passed in via CMark_DIR is the right approach. However, be aware that some of us build with it unified (all of the toolchain in a single cmake invocation).

Migrating from build-script-impl to build script would be a good learning experience though, it would be welcome and good to see others get involved in the build cleanup efforts.

[MaxDesiatov]

The unified/standalone builds is something that I saw references to in CMake files, but I still don't understand pros/cons and use cases of those. Would you elaborate please why some use unified builds then and how exactly that differs? I'd be happy to add that to docs/BuildManifesto.md or a separate doc file to bring the importance of the improvements to the build system to a wider audience.

[davezarzycki]

Well, different people have different reasons to use unified builds. If you have enough CPU cores, then it's dramatically faster than the build-script.

[MaxDesiatov]

How do you even run it? I'm not able to find any documentation whatsoever about unified builds 🤔

[MaxDesiatov]

If you have enough CPU cores, then it's dramatically faster than the build-script.

BTW, how many CPU cores is enough? 🙂

[davezarzycki]

I don’t know how many cores are enough to appreciate the advantage of unified builds. I have 48 cores and (from memory), it’s about 50% faster than using the build script.

I also don’t know how others setup their unified builds, but for me, I just build LLVM and add swift, etc as external projects (as opposed to internal projects like clang, lld, lldb, etc)

[compnerd]

The unified/standalone builds is something that I saw references to in CMake files, but I still don't understand pros/cons and use cases of those.

IMO the pros is that its a faster build (with my 4 core machine, it ends up being ~10-15% faster). It also reduces the complexity of the build IMO, because the single build directory contains nearly everything (llvm, clang, lld, lldb, swiftc, and the swift runtime/standard library). The last pro IMO is that when you update the repositories, it ensures that the dependencies fully percolate through the entire system.

As to the cons - largely that you get a single configuration type (release vs debug for the entire toolchain). The other cons include things like not being able to do a partial incremental build (e.g. if you update and LLVM has changed, you will need to rebuild LLVM, you cannot go behind its back and do just the incremental swift build).

I also don’t know how others setup their unified builds, but for me, I just build LLVM and add swift, etc as external projects (as opposed to internal projects like clang, lld, lldb, etc)

@davezarzycki I dislike the setup I have currently in that I have swift checked out not where I would like. In theory, you should be able to use LLVM_EXTERAL_<PROJECT>_SOURCE_DIR to avoid the checkout layout that I have (where swift is next to clang). But, I basically build llvm with clang, lld, lldb, cmark, and swift as a single build.

[shahmishal]

Please update the base branch to main by Oct 5th otherwise the pull request will be closed automatically.

• How to change the base branch: (Link)
• More detail about the branch update: (Link)