hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.
================
Comment at: llvm/cmake/modules/AddLLVM.cmake:1640
+ # Like LLVM_{TOOLS,LIBS}_DIR, but pointing at the build tree.
+ string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" CURRENT_TOOLS_DIR
"${LLVM_RUNTIME_OUTPUT_INTDIR}")
+ string(REPLACE "${CMAKE_CFG_INTDIR}" "${LLVM_BUILD_MODE}" CURRENT_LIBS_DIR
"${LLVM_LIBRARY_OUTPUT_INTDIR}")
----------------
sammccall wrote:
> hokein wrote:
> > sammccall wrote:
> > > hokein wrote:
> > > > the change looks good, but I'm not sure the new name
> > > > `CURRENT_TOOLS_DIR` is good, the name seems vague and not significantly
> > > > better, since it always points at the build tree, maybe
> > > > `LLVM_BUILD_DIR` or something similar? (The existing naming pattern
> > > > looks like project-specific variables are prefixed with the project
> > > > name `LLVM_`, `CLANG_`).
> > > I agree the new name isn't good. (OTOH *all* these variables seem to have
> > > terrible names, so maybe it's just hard). But I think `LLVM_BUILD_DIR` is
> > > worse.
> > >
> > > The name needs to communicate a lot of things:
> > > 1. that this is parallel to `LLVM_TOOLS_DIR`, and the successor to
> > > `$PROJECT_TOOLS_DIR`
> > > 2. that when LLVM and $PROJECT are built in different paths, this is the
> > > dir for $PROJECT and not for LLVM
> > > 3. That this is not a concrete directory, but may contain a
> > > `%(build_mode)%` placeholder
> > > 4. this variable is part of llvm-project, as opposed to some other CMake
> > > project it may be mixed with.
> > >
> > > `CLANG_TOOLS_DIR` passes 1, 2, 4 (and fails 3, which is really hard).
> > > `CURRENT_TOOLS_DIR` passes 1 & 2 only - it lacks namespacing.
> > > `LLVM_BUILD_DIR` passes 4 only, I think - it's very unclear how it
> > > relates to `LLVM_TOOLS_DIR`.
> > > `LLVM_CURRENT_TOOLS_DIR` would pass 1 and 4, but muddies the
> > > LLVM-vs-subproject distinction a bit.
> > >
> > > What do you think of these criteria, are there some I missed? Any more
> > > name ideas? :-)
> > > The name needs to communicate a lot of things:
> >
> > > 1. that this is parallel to LLVM_TOOLS_DIR, and the successor to
> > > $PROJECT_TOOLS_DIR
> > > 2. that when LLVM and $PROJECT are built in different paths, this is the
> > > dir for $PROJECT and not for LLVM
> > > 3. That this is not a concrete directory, but may contain a
> > > %(build_mode)% placeholder
> > > 4. this variable is part of llvm-project, as opposed to some other CMake
> > > project it may be mixed with.
> >
> > Thanks, these make sense, I were not aware of all of these and didn't get
> > the full picture. I suppose it is being used for exposing various
> > `config.{project}_tools_dir` variables for different tools (e.g.
> > `config.clang_tools_dir`, `config.lldb_tools_dir` etc).
> >
> > > CURRENT_TOOLS_DIR passes 1 & 2 only - it lacks namespacing.
> >
> > hmm, I think the new name is somewhat weak on 2 (particularly the PROJECT
> > bit) -- the `Current` was unclear to me, I guess it means the current
> > project right? I think it'd be better to have the PROJECT in the variable
> > name, `LLVM_PROJECT_TOOLS_DIR` or shorter `PROJECT_TOOLS_DIR`. WDYT?
> >
> > > The name needs to communicate a lot of things:
> >
> > > 1. that this is parallel to LLVM_TOOLS_DIR, and the successor to
> > > $PROJECT_TOOLS_DIR
> > > 2. that when LLVM and $PROJECT are built in different paths, this is the
> > > dir for $PROJECT and not for LLVM
> > > 3. That this is not a concrete directory, but may contain a
> > > %(build_mode)% placeholder
> > > 4. this variable is part of llvm-project, as opposed to some other CMake
> > > project it may be mixed with.
> >
> > Thanks, these make sense, I were not aware of all of these and didn't get
> > the full picture. I suppose it is being used for exposing various
> > `config.{project}_tools_dir` variables for different tools (e.g.
> > `config.clang_tools_dir`, `config.lldb_tools_dir` etc).
>
> That's right. Ultimately it's being used to make sure that when you run lit
> tests, `clang` resolves to the clang you just built and not something else.
> (It's more complicated than that, there are several conflicting mechanisms,
> but that's the essence).
>
> > > CURRENT_TOOLS_DIR passes 1 & 2 only - it lacks namespacing.
> >
> > hmm, I think the new name is somewhat weak on 2 (particularly the PROJECT
> > bit) -- the `Current` was unclear to me, I guess it means the current
> > project right?
>
> It really means the current cmake invocation.
>
> In a "standalone build" of clang (rooted at llvm-project/clang, with a
> separate LLVM tree), it is `build/bin` where `LLVM_TOOLS_DIR` is
> `path/to/llvm/bin`, and was produced by a *previous* cmake invocation (or an
> install step).
>
> In a normal unified build of llvm+clang+lld (rooted at llvm-project/llvm with
> LLVM_ENABLE_PROJECTS), both variables are set to `build/bin`.
>
> There's really no fixed connection with a subproject at all, and certainly
> its value doesn't vary across the various subprojects of llvm-project.
>
> > I think it'd be better to have the PROJECT in the variable name,
> > `LLVM_PROJECT_TOOLS_DIR`
> I think this name is misleading because of the connection to the repo
> `llvm-project`.
> It suggests `LLVM_PROJECT_TOOLS_DIR` is a top-level thing when it's really
> the opposite, a subproject thing. (And while `LLVM_TOOLS_DIR` *does*
> correspond to `llvm-project/llvm` the tools are often things people think of
> as "top-level").
>
> > or shorter `PROJECT_TOOLS_DIR`. WDYT?
>
> I can live with it, but I'm not really sure whether `PROJECT` communicates
> anything that's true :-)
> I'm tempted by just `TOOLS_DIR` but I worry that such a short name with a
> lack of namespacing is asking for trouble. On the other hand, there's
> `SHLIBDIR`, which also points at shared libs in the current build tree...
Thanks for the explanations, now I'm convinced, Let's stick with the current
name :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121763/new/
https://reviews.llvm.org/D121763
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
