Move update_cc_test_checks.py tests to clang

[arichardson]

Summary

Having tests that depend on clang inside llvm/ are not a good idea since
it can break incremental ninja check-llvm.

Fixes https://llvm.org/PR44798

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: pass.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[lebedev.ri]

Honestly, i very much don't like this.
I think that update tool does not belong in the llvm subrepo in the first place, it should be in clang.

[arichardson]

In D74051#1859724, @lebedev.ri wrote:

Honestly, i very much don't like this.
I think that update tool does not belong in the llvm subrepo in the first place, it should be in clang.

I agree that probably makes sense. @MaskRay what do you think?

[MaskRay]

I agree with @lebedev.ri

llvm should not depend on other subprojects.

[arichardson]

Move tests to clang/ instead

[lebedev.ri]

LGTM, i guess
Thank you.

[MaskRay]

@rsmith Does clang/test/utils/ (a new directory) look appropriate to you?

[rsmith]

In D74051#1872856, @MaskRay wrote:

@rsmith Does clang/test/utils/ (a new directory) look appropriate to you?

That seems fine to me. Following that pattern, I think we should also move:

• test/clang-rename -> test/tools/clang-rename
• test/ClangScanDeps -> test/tools/clang-scan-deps
• test/TableGen -> test/utils/TableGen

(We might want to move part of test/Index to test/tools/libclang and parts to test/tools/c-index-test, but we'd need to decide for each test whether it's a test for Clang's behavior, a test for libclang's wrapping behavior, or a test for the c-index-test tool itself. Let's leave that alone for now.)

[thakis]

Fails on windows, looks like line endings: http://45.33.8.238/win/8350/step_7.txt

[arichardson]

In D74051#1876291, @thakis wrote:

Fails on windows, looks like line endings: http://45.33.8.238/win/8350/step_7.txt

Are the other update_test_checks.py and update_llc_test_checks.py tests running on that bot? I would expect them to fail in the same way otherwise?

[arichardson]

Turns out those scripts use different code to write the updated file. The failure should hopefully be fixed in rGc29310707e9a85e70a226277657cd9d9182a5d04

[thakis]

Yep, thanks!

[mgorny]

This broke running clang tests stand-alone:

Traceback (most recent call last):
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang-abi_x86_64.amd64/bin/../../llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang/test/utils/update_cc_test_checks/lit.local.cfg", line 21, in <module>
    assert os.path.isfile(script_path)
AssertionError
                                                           
FAILED: test/CMakeFiles/check-clang

Obviously this fails when LLVM source tree is not available. If this is used only in clang, the script should be moved to clang as well.

[lebedev.ri]

In D74051#2016078, @mgorny wrote:

This broke running clang tests stand-alone:

Traceback (most recent call last):
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang-abi_x86_64.amd64/bin/../../llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang/test/utils/update_cc_test_checks/lit.local.cfg", line 21, in <module>
    assert os.path.isfile(script_path)
AssertionError
                                                           
FAILED: test/CMakeFiles/check-clang

Obviously this fails when LLVM source tree is not available.

Is that a supported build mode for clang?

If this is used only in clang, the script should be moved to clang as well.

I was under impression that it is exactly what this change did.

[mgorny]

In D74051#2016085, @lebedev.ri wrote:

In D74051#2016078, @mgorny wrote:

This broke running clang tests stand-alone:

Traceback (most recent call last):
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang-abi_x86_64.amd64/bin/../../llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang/test/utils/update_cc_test_checks/lit.local.cfg", line 21, in <module>
    assert os.path.isfile(script_path)
AssertionError
                                                           
FAILED: test/CMakeFiles/check-clang

Obviously this fails when LLVM source tree is not available.

Is that a supported build mode for clang?

Yes, and I don't see why that would change today.

If this is used only in clang, the script should be moved to clang as well.

I was under impression that it is exactly what this change did.

My bad. In that case, the path needs to be updated to refer to clang source tree and not llvm's.

[arichardson]

In D74051#2016101, @mgorny wrote:

In D74051#2016085, @lebedev.ri wrote:

In D74051#2016078, @mgorny wrote:

This broke running clang tests stand-alone:

Traceback (most recent call last):
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang-abi_x86_64.amd64/bin/../../llvm/utils/lit/lit/TestingConfig.py", line 89, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/var/tmp/portage/sys-devel/clang-11.0.0.9999/work/x/y/clang/test/utils/update_cc_test_checks/lit.local.cfg", line 21, in <module>
    assert os.path.isfile(script_path)
AssertionError
                                                           
FAILED: test/CMakeFiles/check-clang

Obviously this fails when LLVM source tree is not available.

Is that a supported build mode for clang?

Yes, and I don't see why that would change today.

If this is used only in clang, the script should be moved to clang as well.

I was under impression that it is exactly what this change did.

My bad. In that case, the path needs to be updated to refer to clang source tree and not llvm's.

The update_cc_test_checks.py script depends on files in llvm/utils/UpdateTestChecks, so moving the script to clang doesn't help.
I assumed that having clang as part of the monorepo meant llvm would be available.
I building a tarball of only the clang source directory is supported, then we should skip the clang/tests/utils/update_cc_test_checks tests subdirectory if LLVM sources are not available.

[mgorny]

Nevermind, I figured out good enough workaround.

[tstellar]

In D74051#2017115, @mgorny wrote:

Nevermind, I figured out good enough workaround.

What was the workaround?

[mgorny]

In D74051#2211869, @tstellar wrote:

In D74051#2017115, @mgorny wrote:

Nevermind, I figured out good enough workaround.

What was the workaround?

Unpack llvm/utils/{UpdateTestChecks,update_cc_test_checks.py} from LLVM while building Clang.

clang/test/utils/update_cc_test_checks/lit.local.cfg

+import os
+
+import lit.util
+
+# python 2.7 backwards compatibility
+try:
+    from shlex import quote as shell_quote
+except ImportError:
+    from pipes import quote as shell_quote
+
+
+config.test_format = lit.formats.ShTest(execute_external=False)
+config.suffixes = ['.test']
+
+clang_path = os.path.join(config.clang_tools_dir, 'clang')
+extra_args = '--clang ' + shell_quote(clang_path)
+opt_path = os.path.join(config.llvm_tools_dir, 'opt')
+extra_args += ' --opt ' + shell_quote(opt_path)
+script_path = os.path.join(config.llvm_src_root, 'utils',
+                           'update_cc_test_checks.py')
+assert os.path.isfile(script_path)
+config.substitutions.append(
+    ('%update_cc_test_checks', "%s %s %s" % (
+        shell_quote(config.python_executable), shell_quote(script_path),
+        extra_args)))

llvm/test/tools/UpdateTestChecks/lit.local.cfg

if os.path.isfile(opt_path):
     add_update_script_substition('%update_analyze_test_checks',
                                  extra_args=opt_arg)
 
 llvm_mca_path = os.path.join(config.llvm_tools_dir, 'llvm-mca')
 if os.path.isfile(llvm_mca_path):
     config.available_features.add('llvm-mca-binary')
     mca_arg = '--llvm-mca-binary ' + shell_quote(llvm_mca_path)
     add_update_script_substition('%update_test_checks', extra_args=mca_arg)
-
-clang_path = os.path.join(config.llvm_tools_dir, 'clang')
-if os.path.isfile(clang_path):
-    config.available_features.add('clang-binary')
-    extra_args = '--clang ' + shell_quote(clang_path)
-    if os.path.isfile(opt_path):
-        extra_args += ' --opt ' + shell_quote(opt_path)
-    add_update_script_substition('%update_cc_test_checks', extra_args=extra_args)

llvm/test/tools/UpdateTestChecks/update_cc_test_checks/lit.local.cfg

-# These tests require clang.
-if 'clang-binary' not in config.available_features:
-    config.unsupported = True