[LNT] Add cflags string argument to lnt (2)

[• chatur01]

Summary

Moving this review onto Phab.

Thanks for your comments Chris. I have added some tests for this feature and the original cflag feature, for which I didn't find existing tests.
To test the command-line argument, I added a bit of hack in nt.py which just add a note with the parsed TARGET_FLAGS.
I then use this note in my regression tests to check that the cflags have been correctly parsed. This approach didn't feel right, but I could see a better way.

Vasileios, I'm happy to add support for that option after this review.

Also added some support for quoted strings. I don't think my approach works on Windows shells in general, but I don't this that's of concern for LNT?

Thanks again for your review.

[• chatur01]

Adding a dummy comment to get this to the list, seems to have got stuck in Phabricator?

[cmatthews]

Otherwise looks good. Thanks for adding a test!

lnt/tests/nt.py

136

Lets do these imports at the top. I assume these are in a standard python 2.7 distro?

139

unix. Don't forget poor little OS X!

[• chatur01]

Thanks Chris.

Yep, they are in the stadnard 2.7 distro module list at https://docs.python.org/2/py-modindex.html

Renamed to unix (guess I should have posix to be even more inclusive?) Sorry about that :^)

Landed r239518.

[jmolloy]

[Phab spring clean] This got accepted.

lnt/tests/nt.py

         # FIXME: Eliminate this blanket option.
         target_flags.extend(self.cflags)
 
+        if self.cflag_string:
+            import shlex
+            import pipes
+            # FIXME: This isn't generally OK on Windows :/
+            safely_quoted_on_linux = map(pipes.quote, shlex.split(self.cflag_string))            
+            target_flags.extend(safely_quoted_on_linux)
+
         # Pass flags to backend.
         for f in self.mllvm:
             target_flags.extend(['-mllvm', f])

         group.add_option("", "--cflag", dest="cflags",
                          help="Additional flags to set in TARGET_FLAGS",
                          action="append", type=str, default=[], metavar="FLAG")
+        group.add_option("", "--cflags", dest="cflag_string",
+                         help="Additional flags to set in TARGET_FLAGS, space separated string. "
+                         "These flags are appended after *all* the individual --cflag arguments.",
+                         type=str, default='', metavar="FLAG")
         group.add_option("", "--mllvm", dest="mllvm",
                          help="Add -mllvm FLAG to TARGET_FLAGS",
                          action="append", type=str, default=[], metavar="FLAG")

         # test-suite directory, that borks things. <rdar://problem/7876418>
         prepare_report_dir(config)
 
+        note('TARGET_FLAGS: {}'.format(' '.join(config.target_flags)))
+
         # Multisample, if requested.
         if opts.multisample is not None:
             # Collect the sample reports.

tests/runtest/nt.py

 # RUN:   --test-suite %S/Inputs/test-suite \
 # RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
 # RUN:   --no-timestamp --without-llvm > %t.log 2> %t.err
+
+# Check cflag handling
+
+## With a lone cflag
+# RUN: lnt runtest nt \
+# RUN:   --sandbox %t.SANDBOX \
+# RUN:   --test-suite %S/Inputs/test-suite \
+# RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
+# RUN:   --cflag '-Wall' \
+# RUN:   --no-timestamp > %t.log 2> %t.err
+# RUN: FileCheck --check-prefix CHECK-CFLAG1 < %t.err %s
+# CHECK-CFLAG1: inferred C++ compiler under test
+# CHECK-CFLAG1: TARGET_FLAGS: -Wall
+
+## With a couple of cflags
+# RUN: lnt runtest nt \
+# RUN:   --sandbox %t.SANDBOX \
+# RUN:   --test-suite %S/Inputs/test-suite \
+# RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
+# RUN:   --cflag '-Wall' \
+# RUN:   --cflag '-mfloat-abi=hard' \
+# RUN:   --cflag '-O3' \
+# RUN:   --no-timestamp > %t.log 2> %t.err
+# RUN: FileCheck --check-prefix CHECK-CFLAG2 < %t.err %s
+# CHECK-CFLAG2: inferred C++ compiler under test
+# CHECK-CFLAG2: TARGET_FLAGS: -Wall -mfloat-abi=hard -O3
+
+## With a cflags
+# RUN: lnt runtest nt \
+# RUN:   --sandbox %t.SANDBOX \
+# RUN:   --test-suite %S/Inputs/test-suite \
+# RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
+# RUN:   --cflags '-Wall -mfloat-abi=hard -O3' \
+# RUN:   --no-timestamp > %t.log 2> %t.err
+# RUN: FileCheck --check-prefix CHECK-CFLAG3 < %t.err %s
+# CHECK-CFLAG3: inferred C++ compiler under test
+# CHECK-CFLAG3: TARGET_FLAGS: -Wall -mfloat-abi=hard -O3
+
+## With a cflags with a quoted space and escaped spaces
+# RUN: lnt runtest nt \
+# RUN:   --sandbox %t.SANDBOX \
+# RUN:   --test-suite %S/Inputs/test-suite \
+# RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
+# RUN:   --cflags "-Wall -test=escaped\ space -some-option='stay with me' -O3" \
+# RUN:   --no-timestamp > %t.log 2> %t.err
+# RUN: FileCheck --check-prefix CHECK-CFLAG4 < %t.err %s
+# CHECK-CFLAG4: inferred C++ compiler under test
+# CHECK-CFLAG4: TARGET_FLAGS: -Wall '-test=escaped space' '-some-option=stay with me' -O3
+
+## With cflag and cflags
+# RUN: lnt runtest nt \
+# RUN:   --sandbox %t.SANDBOX \
+# RUN:   --test-suite %S/Inputs/test-suite \
+# RUN:   --cc %{shared_inputs}/FakeCompilers/clang-r154331 \
+# RUN:   --cflag '--target=armv7a-none-eabi' \
+# RUN:   --cflag '-Weverything' \
+# RUN:   --cflags '-Wall -test=escaped\ space -some-option="stay with me" -O3' \
+# RUN:   --no-timestamp > %t.log 2> %t.err
+# RUN: FileCheck --check-prefix CHECK-CFLAG5 < %t.err %s
+# CHECK-CFLAG5: inferred C++ compiler under test
+# CHECK-CFLAG5: TARGET_FLAGS: --target=armv7a-none-eabi -Weverything -Wall '-test=escaped space' '-some-option=stay with me' -O3