Help with patch D101639

Hi everyone,
this is the first patch I've submitted to LLVM, and I'm not well
acquainted with the automated scripts that are run as pre-merge tests.

I've been able to solve the formatting related issues, but there are a
couple of tests that I'm not sure what to do about: clang-tidy warns
about missing header files, likely caused by the OCaml headers not being
present on the machine running pre-tests, and a suggested fix in case of
"unhelpful" clang-tidy warnings is to put the file into the ignorelist.
I'm not sure this is the right course of action, because obviously this
file has worked before without having to ignore it.

Also, the 'lit.lit::test-output-micro.py' completely stumps me, it
doesn't seem to be related to what I'm patching and I can't really
understand its result.

This is the link to phabricator for reference:
https://reviews.llvm.org/D101639

Thanks and best regards,
Francesco

Hi Francesco,

clang-tidy will run over all files in a patch, even if not supported in
the build system, so errors about missing header files can often be
ignored.

As for the failing lit test, the are dependent on trunk having no build
failures, which unfortunately isn't always the case. If the failing
test doesn't appear to be related to the code you are working on in any
way, it can (usually) be ignored.

~Nathan

Unfortunately I believe that until we have a buildbot that is continuously building the main branch with the exact same configuration as the pre-merge, we’ll keep having this issue of frequent unrelated failure. I’ve seen some tests being broken for days (and over a week) in the premerge configuration sometimes, likely because they don’t have coverage on any of the post-commit bots.

On the more general point of unrelated pre-merge failures. I’ve hit these a few times and also find them frustrating. Especially as a new contributor, it’s really hard to know which checks are “ok to ignore”. I find this even with things that are more like lint than full builds (e.g. clang-tidy). Having pre-merge checks is super valuable of course, but I think generally they should be less strict than post-submit.

I’m not super familiar with the infra for these in particular, but I’m a bit surprised that this is a problem. Usually when setting up bots for my project I turn them on for post-submit before turning them on for pre-submit. Is there a reason that the infrastructure can’t be shared here such that presubmit and postsubmit are basically just switches on the exact same build configuration? I gather that the phab integration adds a fair amount of complexity there. Would be a point in favor of using GitHub PRs I guess (although the review ux is definitely nicer on phab IMO)

Thanks everybody for clearing that up anyways, I was getting concerned I
had done something so dumb even the error messages were confused :slight_smile:

Francesco