Thanks for catching! I suspect this might explain why some of the refactors @phosek and I were doing before did not work.

Oh hah! Thanks for catching.

Thanks for doing these comments! That is the sort of thing I wanted for this.

Also, how about all this code goes to kmp_affinity.h? Then we dodge the problem entirely.

@tianshilei1992 Oh sorry you were talking about https://cmake.org/cmake/help/latest/prop_tgt/INTERFACE_INCLUDE_DIRECTORIES.html#prop_tgt:INTERFACE_INCLUDE_DIRECTORIES that behavior right? Specific to when target_link_library is used to depend on a target and not, say, an external preexisting library file.

Hmm. Do you have docs for this, I would like for you to be right? https://cmake.org/cmake/help/latest/command/target_link_libraries.html only mentions macOS framework includes.

Ah, but add_dependency doesn't have a private version. OK, fine.

We can land this simple one now, but I really don't like doing stop-gap fixes on development branches instead of fixing the underlying issue. The fact of the matter is, if tech debt isn't fixed as prompted by bugs, it usually never gets fixed at all.

Looks like kmp,h is not installed. OK this will call for something fancier.

@tianshilei1992 is right. Basically the older CMake stuff like include_directories only worked by side effects, and therefore is impossible to use correctly and modulary and is just generally terrible. the new target stuff is much better, and actually allows tracking who depends on what.

@s-sajid-ali Thanks! Your call, but might you want to make the other ones target_include_directories(..., PRIVATE) while you are at it?

 $ git grep hwloc.h
openmp/README.rst:  ``hwloc.h`` in ``${LIBOMP_HWLOC_INSTALL_DIR}/include`` and the library in
openmp/runtime/cmake/config-ix.cmake:  check_include_file(hwloc.h LIBOMP_HAVE_HWLOC_H)
openmp/runtime/src/kmp.h:#include "hwloc.h"

was useful to me.

LIBOMP_INCLUDE_DIR is a weird name because aren't we building libomp? That should be a directory were headers are build/installed not where they are gotten from. For hermetic builds, the distinction of locations to read vs locations to write is very important.

git archive --help says:

I wanted to check this is syntactically identical to Clang's, so we had a clear pattern going forward.

Could one of you review this?

Could one of you review this?

@nikic I have tried to address your concerns in https://reviews.llvm.org/D118792

In D116492#3290087, @nikic wrote:

Would it be possible to share a complete new-style cmake invocation that is supposed to work?

Hmm, might these errors be legitimate?

Oh yes oops. It had the \${CMAKE_INSTALL_PREFIX}/ before, so I just put that back right the way it was.

Fix more properly

https://lab.llvm.org/buildbot/#/builders/30/builds/16850 this is causing a build failure because llvm/docs/GettingInvolved.rst still links the old proposal.

Rebase to retrigger CI now that parent landed

Oops changed too much

I'd update this change to move llvm/cmake/modules/LLVMCheckLinkerFlag.cmake to cmake/Modules/CheckLinkerFlag.cmake

@phosek does that reasoning check out to you?

In D117537#3266889, @phosek wrote:

What's the reason for keeping both functions instead of changing llvm_check_linker_flag to use CMAKE_REQUIRED_FLAGS?

Alright, this is the last bit (except for D101070 which evidently caused the issues), looking good.

Trim down, now that many parts have been landed already

This part, too, checks out. Landing next.

Fix more issues

Fix includes

1. Updating D117833: [compiler-rt][libunwind][runtimes] Recategorize llvm_check_linker_flag langs #
2. Enter a brief description of the changes included in this update.
3. The first line is used as subject, next lines as comment. #
4. If you intended to create a new revision, use:
5. $ arc diff --create

rebase after last round changed

Restore compromise a bit more

Fix and simplify CMAKE_REQUIRED_DEFINITIONS

rebase on top of new version of D117537

New attempt splitting the difference between last 2

I will land this next as part of my great D99484 bisect. Now that I have in fact found the issue, I am more confident this part, which is analogous to all the parts that have gone in fine, is harmless.

Thanks @dalteny for investigating. @phosek if you remember https://reviews.llvm.org/rG6e3946c9f558adfddfd98a51721baccd8b17bb85 the TODO you put in there about the compiler-rt build having to be standalone seems related.

Remove the LLVM part, which is now D101070, and furthermore known to cause issues.

This seems to be the part of D99484 that wasn't working.

Continuing my bisect of D99484 (since this change was in there at the time it was approved, but failing) I will land this one next.

I have been auto-emailed with some build failures, but they seem unrelated (see https://reviews.llvm.org/rG60f61918795b91f3fc9d310bc0957b75783f826b) so haven't taken any action doing a revert yet.

Perhaps the difference betwen CMAKE_EXE_LINKER_FLAGS and CMAKE_REQUIRED_FLAGS is important? Also, if it is not possible to link until these libraries are built (e.g. in a cross setting) maybe I am not sure what happens.

Much simpler now

@phosek I agree with your fixing what language to do each flag, but do note I was purposely matching the existing dubious categorizations. Might it be better to first land this as-is, and then make a separate patch to clean up the languages?

Fix STREQUAL which is actually infix

Fix remaining missing C/CXX arguments

Rebase after D117617 landed