[Lldb-commits] [PATCH] D43096: [lit] Update how clang and other binaries are found in per-configuration directories

Fri Feb 9 08:13:35 PST 2018

If the default is already to use the just built clang, and an explicit path
allows you to override this, then do we need any switch at all? If the
problem is that the default doesn’t correctly handle visual studio
generator, maybe we could fix that instead?
On Thu, Feb 8, 2018 at 9:43 PM Stiliyana Stamenova <
stiliyana.stamenova at gmail.com> wrote:

> CMAKE_CFG_INTDIR actually evaluates to "." when using make or Ninja, or
> literally the string $(Configuration) when using VS at the point at which
> the lit configuration file is being created. I think that's true for other
> tools as well:
> https://cmake.org/cmake/help/v3.10/variable/CMAKE_CFG_INTDIR.html
>
> If CMAKE_CFG_INTDIR evaluated to Release or Debug, we would not need the
> substitution at all because the path to the tools would be correct already,
> but at the point at which we're looking at CMAKE_CFG_INTDIR in the lit
> configuration file, the path to the just-built compiler gets set to
> something like: "/path/to/llvm/builds/$(Configuration)/bin/clang", so when
> we try to run the tests, the compiler is not found.
>
> That said, adding a new property to make the choice explicit is simple
> enough. Currently, the default is to use the just-built compiler (unless a
> path is specified), so we could add LLDB_TEST_JUST_BUILT_CLANG set to ON by
> default to keep the current default settings.
>
> On Thu, Feb 8, 2018 at 9:07 PM, Zachary Turner <zturner at google.com> wrote:
>
>> The problem is that CMAKE_CFG_INTDIR evaluates to something very common,
>> like “Debug” or “Release”, and replacing a common string like that in a
>> path creates more problems than it solves in my opinion. What if your build
>> dir is C:\src\LLDBDebugger\out\Debug\clang.exe? Debug appears twice in this
>> string so a replace might have unexpected results.
>>
>> More importantly though, what if you *dont* want to use the just built
>> compiler? Maybe you specifically want to use the one from the release dir
>> because it’s faster, but your lldb is in the debug dir?
>>
>> What if we say that if LLDB_TEST_JUST_BUILT_CLANG is ON, it initializes
>> these values as you’re doing now, but if you specify them manually it uses
>> exactly what you specify with no replacement? Would that work? (Having one
>> variable also saves some typing on the command line anyway)
>>
>> On Thu, Feb 8, 2018 at 8:24 PM Stella Stamenova via Phabricator <
>> reviews at reviews.llvm.org> wrote:
>>
>>> stella.stamenova added inline comments.
>>>
>>>
>>> ================
>>> Comment at: lit/CMakeLists.txt:10-13
>>> +string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE}
>>> LLDB_TEST_C_COMPILER ${LLDB_TEST_C_COMPILER})
>>> +string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE}
>>> LLDB_TEST_CXX_COMPILER ${LLDB_TEST_CXX_COMPILER})
>>> +string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_LIBS_DIR
>>> ${LLVM_LIBRARY_OUTPUT_INTDIR})
>>> +string(REPLACE ${CMAKE_CFG_INTDIR} ${LLVM_BUILD_MODE} LLDB_TOOLS_DIR
>>> ${LLVM_RUNTIME_OUTPUT_INTDIR})
>>> ----------------
>>> zturner wrote:
>>> > This only works if you're using a just-built clang, which might not be
>>> the case.  In fact, it's usually not the case, because it's common to want
>>> to run the test suite against a debug build of lldb but using a release
>>> build of clang (otherwise you'll be there all day waiting for it to finish).
>>> >
>>> > I feel like if the user specifies an absolute path to the test
>>> compiler on the command line, that should be what it uses -- always.  If we
>>> want to use a just built toolchain, maybe we need something else, like
>>> `-DLLDB_TEST_BUILT_CLANG=ON`, which would trigger this logic.
>>> >
>>> > As I don't use this configuration though, I'm interested in hearing
>>> your thoughts.
>>> It actually does work in the case when a user specifies a compiler on
>>> the command line as well as when the just-built clang is used and the
>>> default today is to use the just-built clang.
>>>
>>> As far as I can tell, you can specify the compiler with
>>> LLDB_TEST_C_COMPILER (or LLDB_TEST_CXX_COMPILER) when calling CMake or by
>>> passing a value to the tests directly with -C. I assume that you are
>>> concerned about the first case - when passing the property to CMake?
>>>
>>> In that case LLDB_TEST_C_COMPILER is set to /path/to/compiler and these
>>> lines won't actually affect it - unless for some reason the path contained
>>> ${CMAKE_CFG_INTDIR}. If ${CMAKE_CFG_INTDIR} is a period, which is the
>>> likely scenario, it will just be replaced by another period since the build
>>> mode would be the same.
>>>
>>> The only case when it might not work is if ${CMAKE_CFG_INTDIR} is in the
>>> path but not a period - but that is unlikely since the scenario would mean
>>> that ${CMAKE_CFG_INTDIR} is, say, $Configuration and the path that the user
>>> specified contains $Configuration AND it is different than the one they're
>>> using for LLDB.
>>>
>>> On the other hand, without this change it is not possible to use the
>>> just-built compiler when using Visual Studio, for example, because the path
>>> to the just built compiler is not being set correctly and this particular
>>> substitution needs to happen here.
>>>
>>> The way to make sure that both scenarios work always is to guard against
>>> the substitution when the user explicitly specifies a path or to enforce
>>> the sibstitution when they're using the just-built clang so a property like
>>> LLDB_TEST_BUILD_CLANG would work. If you think that the error scenario is
>>> likely enough, then we should add the property.
>>>
>>>
>>>
>>> https://reviews.llvm.org/D43096
>>>
>>>
>>>
>>>
>
>
> --
> Thanks,
> -Stella
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20180209/dcbb15e5/attachment.html>