This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Add llvm-mt and llvm-rc to Clang bootstrap dependency
ClosedPublic

Authored by haowei on Jan 31 2023, 3:39 PM.

Details

Summary

This patch adds llvm-mt and llvm-rc to the Clang bootstrap dependency when building the Clang under Windows so they can be used to build the Windows builtins and runtimes for the multi stage builds.

Diff Detail

Event Timeline

haowei created this revision.Jan 31 2023, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 3:39 PM
Herald added a subscriber: abrachet. · View Herald Transcript
haowei requested review of this revision.Jan 31 2023, 3:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2023, 3:39 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
phosek added inline comments.Feb 1 2023, 1:54 AM
clang/cmake/caches/Fuchsia.cmake
42–52 ↗(On Diff #493755)

You should be able to use the following to avoid having to separate the variables by type:

foreach(variable ${PASSTHROUGH_VARIABLES})
  get_property(is_value_set CACHE ${variable} PROPERTY VALUE SET)
  if(${is_value_set})
    get_property(value CACHE ${variable} PROPERTY VALUE)
    get_property(type CACHE ${variable} PROPERTY TYPE)
    set(BOOTSTRAP_${variable} "${value}" CACHE ${type} "")
  endif()
endforeach()
haowei updated this revision to Diff 494364.Feb 2 2023, 10:44 AM
haowei marked an inline comment as done.
phosek added inline comments.Feb 3 2023, 10:20 AM
clang/cmake/caches/Fuchsia.cmake
20–46 ↗(On Diff #494364)

Could we move this to a separate change since it's unrelated to the llvm-rc and llvm-mt change?

170–171 ↗(On Diff #494364)

Could we do this in https://github.com/llvm/llvm-project/blob/98f0e4f611b40c902cb0df3ef080ae2c00e862d4/clang/CMakeLists.txt#L619? That would be more consistent with other tools such as llvm-ar.

haowei marked 2 inline comments as done.Feb 3 2023, 3:24 PM
haowei added inline comments.
clang/cmake/caches/Fuchsia.cmake
170–171 ↗(On Diff #494364)

I add it into clang/CMakeList.txt file.
the llvm-rc and llvm-mt will only be added to bootstrap tool list on Windows. As they will be required to build Windows runtime for the bootstrap toolchain. On Linux, the runtime for the bootstrap toolchain is Linux only so they are not required.

phosek accepted this revision.Feb 3 2023, 11:02 PM

LGTM but can you update the change title and description since the original one no longer matches the content of the patch.

clang/CMakeLists.txt
599–600

Nit: spelling suggestion.

603–604

Nit: I'd omit this since it doesn't provide any additional information.

This revision is now accepted and ready to land.Feb 3 2023, 11:02 PM
haowei updated this revision to Diff 495221.Feb 6 2023, 11:27 AM
haowei marked an inline comment as done.
haowei retitled this revision from [Fuchsia] Add llvm-mt and llvm-rc to clang bootstrap dependency to [Clang] Add llvm-mt and llvm-rc to Clang bootstrap dependency.
haowei edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Feb 6 2023, 11:37 AM
This revision was automatically updated to reflect the committed changes.