Update description

Switch to clang-library-headers pending cfe-dev discussion

In D58317#1404961, @phosek wrote:

In D58317#1400223, @smeenai wrote:

I'm not entirely happy with the name clang-dev-headers, and am open to suggestions. It's unfortunate clang-headers was already taken for something different, but renaming that target or increasing its scope seems bad for existing users. Other possibilities I thought of include clang-tooling-headers, though that might be confused with the headers for libTooling specifically, and clang-library-headers. I'm open to suggestions.

We could consider renaming clang-headers to e.g. clang-resource-headers and then reusing the name which would match llvm-headers. What do you think about that?

Ah, I didn't realize there were existing uses of DEPS, but thanks for the rename :)

LGTM

I'm not entirely happy with the name clang-dev-headers, and am open to suggestions. It's unfortunate clang-headers was already taken for something different, but renaming that target or increasing its scope seems bad for existing users. Other possibilities I thought of include clang-tooling-headers, though that might be confused with the headers for libTooling specifically, and clang-library-headers. I'm open to suggestions.

LGTM

Comment

In D58284#1399446, @lebedev.ri wrote:

In D58284#1399443, @smeenai wrote:

CC @lebedev.ri, since I believe you raised some issues during the corresponding LLVM change but those were addressed.

Yeah, i don't think i have any comments here.
The logic to pick the default of LLVM_ENABLE_IDE (based on the existence of CMAKE_CONFIGURATION_TYPES) seems hacky,
but it indeed does not currently detect QtCreator's CMake integration as "IDE", so i don't have any additional concerns,
this looks like a straight-forward cleanup.

What i do have concerns about, is that LLVM_ENABLE_IDE is not documented in https://llvm.org/docs/CMake.html

CC @lebedev.ri, since I believe you raised some issues during the corresponding LLVM change but those were addressed.

A couple of things I noticed while writing this patch:

Fix condition

LGTM. There's more clean up that could be done here, but this addresses the immediate issue. Thanks!

LGTM

This could also be improved by teaching clang to use LLVM_BUILD_MAIN_SRC_DIR when that's available.

In D58132#1396724, @pcc wrote:

My personal opinion: 99% of the time I reckon users will want to see the demangled name (it's only us toolchain folks who might more often prefer the mangled names), and there's already a flag --no-demangle which can be used to suppress demangling if necessary. So I think I'd prefer making the COFF linker more like the ELF linker (i.e. don't show mangled names without a flag).

In D58157#1395716, @rnk wrote:

I think we have consensus, so I'll go ahead and stamp it. I don't see any zorg builders that this will affect, I think it will only affect developers... I guess just send a PSA to llvm-dev after you land it.

Ping.

Can you also add clang-tools-extra to LLVM_ALL_PROJECTS?

In D58132#1395055, @ruiu wrote:

It looks like always showing both mangled and demangled names makes the output a bit too large. I'm a little skeptical how useful it is. Where did you find it useful?

LGTM

I think the commit message could be clarified a bit. Something like

I'll give @ruiu a bit to commit this, since he reviewed it, otherwise I can take care of that for you.

Do you need someone to commit this for you?

@mgorny mentioned a need for this to be cached in D57406#1376170, so I'll let him weigh in on this.

LGTM, thanks! I think this makes more sense than before.

In D57535#1388620, @delcypher wrote:

In D57535#1387792, @mehdi_amini wrote:

Could we sanitize the list of LLVM_ENABLE_PROJECTS and error for unknown entries?

I'm fine with this. @smeenai are you doing this or would you like me to put up a patch for this?

@phosek and I discussed this on IRC, and I'm summarizing our discussion here. We're not sure if it actually makes sense for the runtimes build to set the *_STANDALONE_BUILD variables, and he was gonna play with that.

LGTM

In rL353374#624722, @jhenderson wrote:

This looks to have broken optimized tablegen builds with VS2015 even when the new switch is enabled. I'm guessing that option isn't passed into the cmake run for generating the optimized tablegen?

LGTM

I'm putting this up for review just in case there's some potential for breakage I'm not seeing. Note that all these directories are present in the old monorepo as well, so it shouldn't break anyone who's still on that.

In D57535#1387661, @lei wrote:

@delcypher I am having a problem with my internal build after this change have gone in. Can you please help to determine what went wrong? Previously I was able to do the following:

$ cmake -G Ninja -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=On '-DLLVM_ENABLE_PROJECTS=clang;llvm;compiler-rt;clang-tools-extra;openmp;lld' -DCMAKE_C_COMPILER=/home/llvm/gcc/7.3.0/bin/gcc -DCMAKE_CXX_COMPILER=/home/llvm/gcc/7.3.0/bin/g++ -DLLVM_BINUTILS_INCDIR=/usr/include $LLVM_SRC
$ ninja -j 20
$ ninja check-libomptarget check-openmp

After your patch, I get the following warning with the the cmake cmd:

CMake Warning:
  Manually-specified variables were not used by the project:
    LIBOMPTARGET_NVPTX_COMPUTE_CAPABILITIES
    LIBOMPTARGET_NVPTX_ENABLE_BCLIB
    OPENMP_ENABLE_LIBOMPTARGET

and:

$ ninja check-libomptarget check-openmp
ninja: error: unknown target 'check-libomptarget

Ah, I just saw your llvm-dev posts. Thanks :)

In D57535#1384729, @delcypher wrote:

In D57535#1383946, @smeenai wrote:

Do you have any follow-up plans here? No pressure; just didn't want to potentially duplicate any work.

I fixed the issue. Thanks for pointing it out.

Do you have any follow-up plans here? No pressure; just didn't want to potentially duplicate any work.

In D57535#1383069, @delcypher wrote:

In D57535#1382059, @smeenai wrote:

In D57535#1382015, @delcypher wrote:

In D57535#1381862, @smeenai wrote:

Would it be equivalent if you changed https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;352949$1007-1009?as=source&blame=off from an option to a FORCEd cache set? The default value is computed based off the presence of the corresponding LLVM_EXTERNAL_*_SOURCE_DIR variable, which will be set for each project in LLVM_ENABLE_PROJECTS.

No it would not be equivalent.

In the case where the user is not using LLVM_ENABLE_PROJECTS, any user provided value for LLVM_TOOL_<PROJECT>_BUILD (e.g. LLVM_TOOL_CLANG_BUILD) would be ignored because it would always get overwritten with ${${canonical_full_name}_BUILD_DEFAULT}. This would break existing user workflows.

The whole point of this patch is to improve the LLVM_ENABLE_PROJECTS workflow without breaking workflows that set the LLVM_TOOL_<PROJECT>_BUILD variables. I'd like to stop LLVM_TOOL_<PROJECT>_BUILD variables from being CMake cache variables at some point so that those variables are no longer user facing. However that is going to require an RFC and is out of scope for this patch.

The branch in question is only taken when you haven't checked out the project in-tree, and I suspect most users of LLVM_TOOL_*_BUILD are using it for in-tree checkouts. Btw, that branch also checks for LLVM_EXTERNAL_*_BUILD variables, which you're also overriding in this patch, but I would hope no one was using those.

My preference would be to stop using the LLVM_TOOL_*_BUILD variables entirely for out-of-tree checkouts, and have that be completely controlled by LLVM_ENABLE_PROJECTS and LLVM_EXTERNAL_PROJECTS instead (which is effectively what this patch does for LLVM_ENABLE_PROJECTS, but in an indirect way). I think the LLVM_TOOL_*_BUILD variables still have value for in-tree checkouts (although with the impending monorepo migration, in-tree checkouts are probably not long for this world anyway, unless the read-only single project mirrors end up happening and then people check out those mirrors in-tree, for whatever reason).

I agree with all of the above but I think we need to communicate this clearly to other developers. My patch is indirect precisely because I want the support for setting the LLVM_TOOL_*_BUILD variables to still work when LLVM_ENABLE_PROJECTS is set.

In D57535#1382015, @delcypher wrote:

In D57535#1381862, @smeenai wrote:

Would it be equivalent if you changed https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;352949$1007-1009?as=source&blame=off from an option to a FORCEd cache set? The default value is computed based off the presence of the corresponding LLVM_EXTERNAL_*_SOURCE_DIR variable, which will be set for each project in LLVM_ENABLE_PROJECTS.

No it would not be equivalent.

In the case where the user is not using LLVM_ENABLE_PROJECTS, any user provided value for LLVM_TOOL_<PROJECT>_BUILD (e.g. LLVM_TOOL_CLANG_BUILD) would be ignored because it would always get overwritten with ${${canonical_full_name}_BUILD_DEFAULT}. This would break existing user workflows.

The whole point of this patch is to improve the LLVM_ENABLE_PROJECTS workflow without breaking workflows that set the LLVM_TOOL_<PROJECT>_BUILD variables. I'd like to stop LLVM_TOOL_<PROJECT>_BUILD variables from being CMake cache variables at some point so that those variables are no longer user facing. However that is going to require an RFC and is out of scope for this patch.

+@hans for the release branch.

Would it be equivalent if you changed https://reviews.llvm.org/diffusion/L/browse/llvm/trunk/cmake/modules/AddLLVM.cmake;352949$1007-1009?as=source&blame=off from an option to a FORCEd cache set? The default value is computed based off the presence of the corresponding LLVM_EXTERNAL_*_SOURCE_DIR variable, which will be set for each project in LLVM_ENABLE_PROJECTS.

Wait, what happens if you call shutil.rmtree with the \\?\ normalized path, instead of dropping down to ctypes?

An alternative would be to just add build to our top-level .gitignore, assuming that's what most people are naming their build directory. @pcc attempted to do something a bit more ambitious in D57400, but just ignoring /build* (as suggested by @jyknight there) shouldn't cause issues, I think.

Still LGTM.

In D57330#1376069, @mehdi_amini wrote:

You can avoid the git status pollution by adding the build directory to .git/info/exclude.

Good to know! Should we include this in the doc?

I might just commit for post-commit review, since @craig.topper weighed in on my main question above.