Page MenuHomePhabricator

Move update_cc_test_checks.py tests to clang
ClosedPublic

Authored by arichardson on Feb 5 2020, 7:56 AM.

Details

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

Diff Detail

Event Timeline

arichardson created this revision.Feb 5 2020, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2020, 7:56 AM

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.

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.

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?

I agree with @lebedev.ri

llvm should not depend on other subprojects.

Move tests to clang/ instead

Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2020, 7:13 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
arichardson retitled this revision from Add a dependency on clang for check-llvm to Move update_cc_test_checks.py tests to clang.Feb 11 2020, 7:14 AM
arichardson edited the summary of this revision. (Show Details)
arichardson added a reviewer: MaskRay.
lebedev.ri accepted this revision.Feb 12 2020, 9:58 AM

LGTM, i guess
Thank you.

This revision is now accepted and ready to land.Feb 12 2020, 9:58 AM
MaskRay accepted this revision.Feb 12 2020, 11:30 AM

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

@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.)

rsmith accepted this revision.Feb 14 2020, 2:17 AM
This revision was automatically updated to reflect the committed changes.

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

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?

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

mgorny reopened this revision.May 1 2020, 11:23 PM

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.

This revision is now accepted and ready to land.May 1 2020, 11:23 PM
mgorny requested changes to this revision.May 1 2020, 11:23 PM
This revision now requires changes to proceed.May 1 2020, 11:23 PM

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.

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.

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 resigned from this revision.May 4 2020, 12:52 AM

Nevermind, I figured out good enough workaround.

This revision is now accepted and ready to land.May 4 2020, 12:52 AM
arichardson closed this revision.May 4 2020, 4:53 AM

Nevermind, I figured out good enough workaround.

What was the workaround?

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.