This is an archive of the discontinued LLVM Phabricator instance.

[test] Split out Annotations from `TestingSupport`
ClosedPublic

Authored by rupprecht on Jan 6 2023, 7:09 PM.

Details

Summary

The Annotations helper class does not have a gtest or gmock dependency, but because it's bundled with the rest of TestingSupport, it gets one. By splitting it out, a target can use it without being forced to use LLVM's copy of gtest.

Diff Detail

Event Timeline

rupprecht created this revision.Jan 6 2023, 7:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 7:09 PM
rupprecht requested review of this revision.Jan 6 2023, 7:09 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 7:09 PM

It seems like the same logic would extend to the CMake build. Could we make the same change there?

rupprecht updated this revision to Diff 488366.Jan 11 2023, 1:34 PM
  • Move annotations to a separate package entirely
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 11 2023, 1:34 PM

It seems like the same logic would extend to the CMake build. Could we make the same change there?

The simplest (only?) way to do that is to have it literally in a separate directory, so I did that. It's a bit large now but mechanical.

rupprecht retitled this revision from [bazel] Split out Annotations from `TestingSupport` to [test] Split out Annotations from `TestingSupport`.Jan 11 2023, 1:36 PM
GMNGeoffrey accepted this revision.Jan 11 2023, 1:48 PM

LGTM (+/- nits), but maybe good to get review from someone more closely associated with the TestingSupport library

utils/bazel/llvm-project-overlay/llvm/BUILD.bazel
4529

I think this comment is now a bit redundant

This revision is now accepted and ready to land.Jan 11 2023, 1:48 PM
rupprecht updated this revision to Diff 488381.Jan 11 2023, 2:15 PM
  • Remove redundant comment
sammccall accepted this revision.Jan 11 2023, 4:04 PM

This looks good, i think it also doesn't belong in Testing/Support because it's not a peer to anything in Support/.

I think you can use PARTIAL_SOURCES_INTENDED to partition directories, but it's unusual and a bit of a hassle. Either way seems fine to me.

gribozavr2 accepted this revision.Jan 11 2023, 4:22 PM
This revision was landed with ongoing or failed builds.Jan 12 2023, 1:41 PM
This revision was automatically updated to reflect the committed changes.