Page MenuHomePhabricator

Move update_cc_test_checks.py tests to clang
ClosedPublic

Authored by arichardson on Wed, Feb 5, 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.Wed, Feb 5, 7:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Feb 5, 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 TranscriptTue, Feb 11, 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.Tue, Feb 11, 7:14 AM
arichardson edited the summary of this revision. (Show Details)
arichardson added a reviewer: MaskRay.
lebedev.ri accepted this revision.Wed, Feb 12, 9:58 AM

LGTM, i guess
Thank you.

This revision is now accepted and ready to land.Wed, Feb 12, 9:58 AM
MaskRay accepted this revision.Wed, Feb 12, 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.Fri, Feb 14, 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