[PATCH] D122335: [clang] Emit crash reproduction as a single tar file

[Alex Brachet via Phabricator via cfe-commits]

abrachet marked 2 inline comments as done.
abrachet added inline comments.


================
Comment at: clang/lib/Driver/Driver.cpp:1434
     StringRef AdditionalInformation, CompilationDiagnosticReport *Report) {
+#ifdef CLANG_CRASH_SAVE_TEMPS
+  constexpr bool SaveReproTemps = true;
----------------
arichardson wrote:
> This could be simplified with `#cmakedefine01` in the config header
Thanks I like that a lot better.


================
Comment at: clang/test/Driver/crash-report-clang-cl.cpp:1
+// UNSUPPORTED: windows
+
----------------
arichardson wrote:
> I think it would be cleaner to add a new feature if tar is available in $PATH instead of ignoring all windows bots. Some might have tar installed.
It's not clear to me what the Windows builders don't like. `tar(1)` is used in some lld tests. My guess is that the tar shipped on Windows doesn't support `--wildcards` which is seemingly necessary here because the tar file will have a random name, and it's member files are prefixed with that name. I don't have easy access to a Windows machine to test that theory though.

I've gone with your suggestion to have an env var. Do you think I should remove `UNSUPPORTED: windows` on all tests or just this one?


================
Comment at: clang/test/Driver/crash-report-clang-cl.cpp:8
 // RUN:     -fcrash-diagnostics-dir=%t -- %s 2>&1 | FileCheck %s
-// RUN: cat %t/crash-report-clang-cl-*.cpp | FileCheck --check-prefix=CHECKSRC %s
-// RUN: cat %t/crash-report-clang-cl-*.sh | FileCheck --check-prefix=CHECKSH %s
+// RUN: tar xOf %t/*.tar --wildcards "*/tmp/crash-report-clang-cl-*.cpp" \
+// RUN:     | FileCheck --check-prefix=CHECKSRC %s
----------------
arichardson wrote:
> Alternatively, if we had an env var/command line option to keep the cpp/sh, we could use that to avoid the dependency on tar in this test.
Sure.


================
Comment at: clang/test/Driver/crash-report-save-temps.c:14
+
+// RUN: false
----------------
arichardson wrote:
> Is this line intentional?
Nope, thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122335/new/

https://reviews.llvm.org/D122335