[PATCH] D121233: [pseudo] Move pseudoparser from clang to clang-tools-extra

Wed Mar 9 13:00:32 PST 2022

aaron.ballman accepted this revision.
aaron.ballman added a comment.

In D121233#3370821 <https://reviews.llvm.org/D121233#3370821>, @sammccall wrote:

> In D121233#3370177 <https://reviews.llvm.org/D121233#3370177>, @aaron.ballman wrote:
>
>> I agreed based on the understanding that the new code would follow the existing conventions, so perhaps the agreement was a bit premature. But, I'm honestly pretty surprised at this significant of resistance to following the same patterns used by (basically) everything else in the project.
>
> I'm not sure I buy the premise ("everything else in the project"). clang-tools-extra isn't really a project: it doesn't have coherent code, goals, or developers.
> Each subproject in llvm-project **except** those under clang-tools-extra has a root directory containing tests etc for the subproject, and makes good use of this!

Yeah, I think we're converging on this point.

>> We're talking about a directory name and where we put tests; what are the technical challenges here? All of the arguments you've presented seem to be stylistic in nature (not that those kinds of issues aren't important to consider), so I feel like I must be missing something.
>
> Right, directory name is a stylistic concern either way AFAICT (redundancy and consistency).

Agreed.

> Location of tests is more complicated, and more important. Experience from clangd:
>
> - A root directory for build system config is important. IIRC the straw that broke the camel's back for rearranging clangd was having a root CMakeLists file to turn it off in one obvious place.
> - The most common development tools (editors, shells, grep, ...) are much harder to use if your working set is several directories rather than one.
> - Linking strangers to the code <https://github.com/llvm/llvm-project/tree/main/clang-tools-extra/clangd> leaves the tests essentially invisible.
> - It clearly defines the boundaries of the project. Before the move, I don't think most contributors could have answered the question "which directories are part of clangd" with confidence. (I couldn't!)
> - On multiple occasions contributors failed to find one of (unit|lit) tests, because once you're looking in nonobvious places you stop when you find anything. So they put tests in the wrong place.
> - The clang-tools-extra layout doesn't provide test targets for subprojects, which is a dealbreaker; it seems people resort to guessing at llvm-lit invocations instead. We were maintaining `check-clangd` in clang-tools-extra/test/CMakeLists.txt as a special snowflake, which is presumably also objectionable (it's 35 lines of CMake).

Thank you for the details here, this helps me understand the concerns a lot better.

>>> If consistency is to be the sine qua non, then I think we should probably leave the code where it is until someone's willing to do the work you described. clang/Tooling/Syntax is the most consistent location for the code, and the one proposed in the original RFC last year.
>>
>> If you don't feel like updating the projects to do a consistent rename, that's totally fine; I didn't intend to imply that was expected of you or required for this to proceed.
>
> When you say it's fine, do you mean that it's acceptable to land this patch, or to abandon it, or to land your preferred version that I'm not OK with? :-)

Sorry for the confusion, I mostly meant the last point, but without any additional work on your end after the patch lands.

> I think we are now is:
>
> - there was an RFC
> - several months later, code started landing, and there was new discussion (on the RFC thread) including a proposal to move
> - we agreed that a move is OK with us, but are stuck on the details
>
> So if it makes a difference, it seems you're _requesting_ a change here, not approving one.

A bit of both, really. I requested the move, you're doing the move, I didn't approve yet because the move did things I wasn't expecting.

>> The directory layout and naming structure has worked well enough thus far
>
> If people finding build/test setup weird and alienating, I think they're more likely to not contribute than to offer clear feedback. I'm not sure how we can tell it works well.
> Anecdotally, I do know that I was put off from contributing to clang-tidy by how hard it was to maintain and debug tests, I've lost significant time when I've needed to do this, and I've seen others struggle with it too.
> But while this has come up many times in conversation, I've never seen it written down. Does open-source have a taboo about whining about stuff being hard? :-)

My measurement for how well it works is how long it's existed without anyone proposing to change it, but that's not really "measuring" so much as "guessing".

>> I think part of the issue here is that clang-tools-extra is a dumping ground for projects, so it isn't itself a "project" (in the same sense of LLVM, Clang, lldb, etc).
>
> I do agree with this perspective, clang-tools-extra is "just a directory".
> This is why it didn't seems like we were doing something wild with clangd: we were just treating it like a "real" subproject under the llvm-project umbrella.
> (I'd be happy with moving clangd or clang-pseudo directly under llvm-project if that addresses the concerns, as long as I don't have to chase impossible consensus-via-RFC).

Yeah, I've been thinking about this a bit more and it's causing me to come around to the idea that the initial structure for clang-tools-extra was wrong (especially coupled with some of your points above).

>> I was originally concerned about the pseudo parser existing in the tree at all because of maintenance burdens, but those concerns were mitigated by moving the project here to clang-tools-extra. I don't think the directory name being `pseudo` instead of `clang-pseudo` will add significant maintenance burden; it'll just be the only project we can't figure out the executable name
>
> The executable we're talking about here is essentially a driver for tests, so this is similar to not being able to find that the name of `clang/Index` or `tools/libclang` is `c-index-test`.

... well shoot, I didn't know that... I thought clang-pseudo was a tool users would run to drive the pseudo parser as a REPL tool (kind of like clang-query) as opposed to a project that exists just to drive tests.

>> But the test directory being in a novel location really does add maintenance burden in the form of making it trivially easy to think you've run tests when you haven't actually done so.
>
> I'm sorry that we broke this workflow, churn is painful. I think it got fixed though!
> It's also a fragile workflow which doesn't update dependencies, which is why we don't use it and hadn't tested it.
> Are you sure this isn't a symptom of relying on low-level tools because `check-clang-tools` is too coarse-grained?

Maybe it is? I've been running llvm-lit manually for so many years, it's hard for me to tell, tbh.

>> This is has already caused me pain with clangd and I don't want to see that being used as precedent to make things worse here.
>
> This feels like a double standard - the precedents you've cited have already caused me pain, too.

Fair point, we're all in pain. :-D Okay, I think you've convinced me of a few things:

1. The name of the tool directory doesn't matter to me any more now that I know this is a test harness and not an executable users are going to be looking for the source for all that often.
2. The layout of clang-tools-extra in terms of test directories is problematic and it'd sure be nice if someday someone moved the test directories under the individual tools instead of using a top-level test directory.
3. Because of #2 and because clangd already started down the path of using a second-level test directory, I'm now okay with the proposed layout in this patch, so giving it my LGTM.

Thank you for bearing with me while we figured this out together!

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121233