It looks like this might not be specific to your change as a few other tests are failing with a similar error. I'm looking into it.

A bunch of the NativePDB tests are failing on Windows now.

In D55607#1334666, @aprantl wrote:

Can you confirm that reverting this path actually fixes the issue? I'm asking because the only test that is executing this script has a REQUIRES: system-darwin line in it.

This change made the Windows test get stuck: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/2340

This does fix the issues. Thanks!

I am trying to use the new builder to build the lldb-mi tests, so that they can start consistently passing on Windows, so they're now using the gcc builder on Linux and failing:

local-variables.cpp is failing on the Buildbot:

Let's use @labath's change instead.

A couple of small comments, but it looks good otherwise. Thanks!

enums-layout.test is now failing on Windows:

LLDB :: BuildScript/toolchain-clang-cl.test is failing on Linux.

One of these tests fail on Windows:

It looks like after this change went in, the tests on Windows starting timing out instead of completing. The last run before the change took about 2 minutes and starting with the first run with this change, they're running for 40 minutes before being killed:

I tested this with your other change and the tests ran successfully. Some of the test code does use single quotes on Windows and double quotes on Linux, though, so it may be worth checking for both regardless of platform.

It works for me when you make the command:

I ran the tests with this change and there are about a dozen new failures:

In D49379#1311309, @kbaladurin wrote:

What is the status? Thank you!

This looks fine, but let's see if Zachary's change lands first and how it looks like after it lands :).

LGTM as long as the tests are all still passing :)

Feel free to check this in. At this point this will only improve the results on Windows and it works correctly on Linux.

That said, in the interest of not changing too much all at once, it still seems like something that's perhaps better done in a future patch. WDYT?

In D54567#1300064, @zturner wrote:

In D54567#1300029, @stella.stamenova wrote:

In D54567#1299999, @zturner wrote:

In D54567#1299993, @stella.stamenova wrote:

In D54567#1299992, @aprantl wrote:

In D54567#1299989, @stella.stamenova wrote:

We should also remove LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER from the cmake files along with this change, otherwise, people will still expect them to work.

That would not be a good idea. There are several bots that are using these flags.

The change that Zachary is making is removing their usage, so after his change they would not do anything. If he ends up committing this change, these two properties (along with LLDB_DEFAULT_TEST_C_COMPILER and LLDB_TEST_USE_CUSTOM_C_COMPILER, etc.) should also go.

The flags are still needed for (and used by) the dotest suite, I didn't change that part. Normally you run that suite by doing ninja check-lldb, in which case it never goes through these lit files to begin with. But they will also run as part of ninja check-lldb-lit, but that lit configuration file totally overrides everything in the parent one, so nothing in this patch should have any effect on that.

I think this is actually confusing - there are two ways to specify compilers for the lldb test suite at cmake time:

1. Via LLDB_TEST_USE_CUSTOM_C_COMPILER and friends
2. Via LLDB_TEST_USER_ARGS
   
   As far as I can tell, the ubuntu 14 bot that @aprantl pointed to uses the LLDB_TEST_USER_ARGS path. I *think* the green dragon bots also use the LLDB_TEST_USER_ARGS and AFAIK it's the only way gcc is ever specified for the tests. If the LLDB_TEST_USE_CUSTOM_C_COMPILER and friends are not used for the lit tests, I don't think we need them for the suite tests.

You're right that it's confusing, but I don't have any good ideas here. Some ideas (which are not necessarily good):

• We could change the names of LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER to LLDB_DOTEST_C_COMPILER and LLDB_DOTEST_CXX_COMPILER.
• We could restructure the directories under lldb/lit so that it looks like this:
  
  ` lldb \- lit |- tests |- unittests \- dotest `
• We could get rid of the lit folder entirely, and put everythign in the tests folder, like this: ` lldb \- test |- lit |- unittests \- dotest `
  
  Maybe some combinatino of these (or other ideas that others have) can make things clearer. But I don't want to change too much all at once.

In D54567#1300030, @aprantl wrote:

In D54567#1300000, @zturner wrote:

http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/

What do I need to click on to get the equivalent of this: http://lab.llvm.org:8011/builders/lldb-x86_64-ubuntu-14.04-cmake/builds/31244/steps/test6/logs/stdio

Click on a recent build and then on "Console Output". Here is one from yesterday (we are upgrading the machines right now and temporarily broke the build):
http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake-clang-5.0.2/1356/consoleFull

In D54567#1299999, @zturner wrote:

In D54567#1299993, @stella.stamenova wrote:

In D54567#1299992, @aprantl wrote:

In D54567#1299989, @stella.stamenova wrote:

We should also remove LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER from the cmake files along with this change, otherwise, people will still expect them to work.

That would not be a good idea. There are several bots that are using these flags.

The change that Zachary is making is removing their usage, so after his change they would not do anything. If he ends up committing this change, these two properties (along with LLDB_DEFAULT_TEST_C_COMPILER and LLDB_TEST_USE_CUSTOM_C_COMPILER, etc.) should also go.

The flags are still needed for (and used by) the dotest suite, I didn't change that part. Normally you run that suite by doing ninja check-lldb, in which case it never goes through these lit files to begin with. But they will also run as part of ninja check-lldb-lit, but that lit configuration file totally overrides everything in the parent one, so nothing in this patch should have any effect on that.

In D54567#1299992, @aprantl wrote:

In D54567#1299989, @stella.stamenova wrote:

We should also remove LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER from the cmake files along with this change, otherwise, people will still expect them to work.

That would not be a good idea. There are several bots that are using these flags.

We should also remove LLDB_TEST_C_COMPILER and LLDB_TEST_CXX_COMPILER from the cmake files along with this change, otherwise, people will still expect them to work.

I took a brief look and I have a question about the usage of clang (rather than clang-cl).

The builds don't specify the two options for LLDB_TEST_C/CXX_COMPILER, so they should be picking up the freshly build compilers. We don't have an option for clang-cl though - so it's never been explicitly specified.

I haven't verified this yet - but I suspect it is now picking up the clang-cl that comes with VS rather than the one that was just built.

In D53530#1290680, @aprantl wrote:

In D53530#1290664, @stella.stamenova wrote:

TestVla fails on Windows: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio

AssertionError: False is not True : 'frame var vla' returns expected result, got '(int []) vla = {}'

I will have time to look in more detail later this week, but if you have any ideas in the mean time, that would be great.

Is your bot using DWARF or CodeView as a debug info format? Because this test definitely requires DWARF.

TestVla fails on Windows: http://lab.llvm.org:8014/builders/lldb-x64-windows-ninja/builds/1129/steps/test/logs/stdio

In D54009#1290644, @zturner wrote:

I have not run the dotest suite recently, is that where you’re seeing the
failures? I successfully ran the lit suite and unit tests though

Several of the windows tests that invoke clang-cl have started failing recently (I am not sure exactly when) and I suspect this change is the culprit. Were you able to run the tests successfully with this change?

Some of the file spec changes caused a bunch of failures on Windows. We know have a silent Buildbot that you can see the failures on as well:

In D54009#1284839, @zturner wrote:

In D54009#1284827, @stella.stamenova wrote:

Looks good. The formatting in lit.cfg.py is a bit messy (indentations, especially), but as long as the tests pass, this is an improvement :). Thanks!

Is there something like clang-format for Python? Happy to fix it if so. (I don't do a lot of Python so it's hard for me to eyeball it and figure out what's wrong, so I have to say it looks fine to me :))

Looks good. The formatting in lit.cfg.py is a bit messy (indentations, especially), but as long as the tests pass, this is an improvement :). Thanks!

In D53158#1277851, @gkistanova wrote:

Hi Stella,

Please see my notes inline.

Unless you want to do this by yourself, I can prepare this change. I think I have developed a good understanding of what you are after.

A couple of comments, but looks good otherwise. I'd wait for someone else to have a look as well.

@gkistanova do you have a concern with the current changes or any additional testing you want me to do before I check this in?

This change fixes the problem on Linux. Thanks!

I updated the patch, so that it will not auto detect the VS installation unless explicitly requested. I also propagated the flag to all the builders that currently work with VS, so that it is available immediately if anyone chooses to create a buildbot with a newer VS installation.