tests/check_also.cc fails on Windows

[GoogleCodeExporter]

I've been investigating why tests/check_also.cc fails on Windows.

======================================================================
FAIL: runTest (__main__.check_also)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "run_iwyu_tests.py", line 106, in <lambda>
    {'runTest': lambda self, f=filename: self.RunOneTest(f)})
  File "run_iwyu_tests.py", line 82, in RunOneTest
    iwyu_flags, verbose=True)
  File "C:\dev\llvm_svn\tools\clang\tools\include-what-you-use\iwyu_test_util.py", line 390, in TestIwyuOnRelativeFile
    test_case.assertTrue(not failures, ''.join(failures))
AssertionError: False is not true :
Unexpected summary diffs for tests/check_also-d1.h:
+++
@@ -1,8 +0,0 @@
-tests/check_also-d1.h should add these lines:
-#include <stddef.h>
-
-tests/check_also-d1.h should remove these lines:
-- #include "check_also-i1.h"  // lines XX-XX
-
-The full include-list for tests/check_also-d1.h:
-#include <stddef.h>  // for NULL

With the getopt_long code from Issue 52, the Windows port now supports -Xiwyu 
--check_also, but the problem seems to be that the paths passed to 
GlobMatchesPath aren't canonicalised, so it's called with these arguments:

path "tests/check_also-d1.h"
glob "tests\*-d1.h"

'glob' ends up with a backslash because of this Python (run_iwyu_tests.py)

def CheckAlsoExtension(extension):
  """Return a suitable iwyu flag for checking files with the given extension."""
  return '--check_also="%s"' % os.path.join(TEST_ROOTDIR, '*' + extension)

I've attached 2 patches which fix this issue in different ways.

(1) Fix run_iwyu_tests.py to canonicalise the paths that are passed to 
--check_also
(2) Fix ShouldReportIWYUViolationsFor and AddGlobToReportIWYUViolationsFor to 
canonicalise paths before they are passed to GlobMatchesPath

There is also a 3rd option which I haven't tried, which is to move the 
canonicalisation to the MSVC implementation of GlobMatchesPath. This is 
slightly less efficient than (2), but will only affect the Windows build.

Are either of these solutions preferable? (2) is probably the most robust 
solution, but if --check_also is only used for testing then (1) is probably 
simplest.

Original issue reported on code.google.com by paul.hol...@gmail.com on 17 Jul 2011 at 12:25

Attachments:

• canonicalise_check_also_args.patch
• canonicalise_glob_and_filepath.patch

[GoogleCodeExporter]

I think this will be the same as issue 54 -- we should be canonicalizing the 
--check_also input in iwyu when we're reading in the pragma (so that's a third 
place where filenames can be input into the system for iwyu).  Let's keep the 
python code alone.

Original comment by csilv...@gmail.com on 19 Jul 2011 at 12:02

[GoogleCodeExporter]

In that case I think this change boils down to the second patch 
(canonicalise_glob_and_filepath.patch)

The paths that are passed on the command-line (via --check_also) need to be 
canonicalised for AddGlobToReportIWYUViolationsFor(). The other place that this 
function is called is IwyuPreprocessorInfo::FileChanged_EnterFile(), and as 
that path also requires canonicalisation I think it makes sense to do this in 
AddGlobToReportIWYUViolationsFor().

That leaves ShouldReportIWYUViolationsFor(), which must canonicalise the path 
that is returned by GetFilePath().

The third place that you mention - the filenames that are specified in pragmas 
- will be cleaned up by the canonicalise_paths.patch in Issue 54 
(ConvertToQuotedInclude() calls NormaliseFilePath() which calls 
CanonicalizeFilePath()).

So I think canonicalise_glob_and_filepath.patch, along with the patch from 
Issue 54, is sufficient to get most of the tests passing on Windows.

Original comment by paul.hol...@gmail.com on 19 Jul 2011 at 10:02

[GoogleCodeExporter]

It looks like only the first diff in canonicalise_glob_and_filepath.patch is 
needed anymore: with the issue-54 patch, GetFilePath() already canonicalizes 
(since it calls NormalizeFilePath()), so the second diff isn't needed.

Feel free to apply the first diff and commit.

Original comment by csilv...@gmail.com on 19 Jul 2011 at 11:11

• Changed state: Started

[GoogleCodeExporter]

Ah, yes - only the first diff was necessary. I've applied this in r279, thanks.

Original comment by paul.hol...@gmail.com on 20 Jul 2011 at 8:34

• Changed state: Fixed