User Details
- User Since
- Mar 16 2018, 2:30 PM (219 w, 19 h)
Thu, May 26
Thanks for catching! I suspect this might explain why some of the refactors @phosek and I were doing before did not work.
Wed, May 25
Apr 20 2022
Oh hah! Thanks for catching.
Apr 15 2022
Thanks for doing these comments! That is the sort of thing I wanted for this.
Mar 30 2022
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.
Mar 29 2022
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.
Mar 18 2022
git archive --help says:
Feb 22 2022
I wanted to check this is syntactically identical to Clang's, so we had a clear pattern going forward.
Feb 19 2022
Could one of you review this?
Could one of you review this?
Feb 3 2022
Feb 2 2022
@nikic I have tried to address your concerns in https://reviews.llvm.org/D118792
Jan 29 2022
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
Jan 28 2022
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
Jan 26 2022
I'd update this change to move llvm/cmake/modules/LLVMCheckLinkerFlag.cmake to cmake/Modules/CheckLinkerFlag.cmake
Jan 25 2022
@phosek does that reasoning check out to you?
Jan 24 2022
Jan 23 2022
Jan 22 2022
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
Jan 21 2022
Fix includes
- Updating D117833: [compiler-rt][libunwind][runtimes] Recategorize llvm_check_linker_flag langs #
- Enter a brief description of the changes included in this update.
- The first line is used as subject, next lines as comment. #
- If you intended to create a new revision, use:
- $ 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.
Jan 20 2022
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?
Jan 19 2022
Fix STREQUAL which is actually infix
Fix remaining missing C/CXX arguments
Rebase after D117617 landed