In D94766#2513512, @jdenny wrote:

In D94766#2513362, @thopre wrote:

In D94766#2513022, @jdenny wrote:

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In our case lit is invoked for each testcase (1 lit invocation per testcase) and testcases are run in parallel. That would require detection before the tests but then you would need a fake test for that because lit does not have a mechanism to ask about whether an option exist except running a known good test with the option.

Yes, that's what I had in mind. It seems simple, but maybe I'm missing something.

A fake test is probably the most robust way to check. But an even simpler check that should work for now is: llvm-lit --help | grep -q -- --no-indirectly-run-check.

But all these are specifics to our setup (we have a workaround anyway), I'm more concerned about having a migration strategy.

What makes it specific to your setup? Feature tests when building/testing a project are common.

In D94766#2513022, @jdenny wrote:

So the lit invocations are in a script of some sort, right? Such a script could perform a feature test to handle this use case: if --no-indirectly-run-check is supported, add it to some lit options variable used by all relevant lit invocations in the script, and do not otherwise. Is that difficult to do?

I'm not strongly opposed to this patch, especially if it helps many people. However, given that this use case is external and temporary, I'm inclined to think a feature test is more reasonable: it's also simple, but it's external like the use case and can be dropped by users when they decide they no longer need to support old lit versions. What do you think?

In D94766#2512681, @jdenny wrote:

Can you explain the use case a bit more? I don't find myself moving between old and new versions of lit very often. Perhaps I'm naive, but it seems like it should be easy enough to just not specify --no-indirectly-run-check when using the old version.

Ping?

In D95033#2509413, @danilaml wrote:

Why does the cache cause this error in mypy? Is it a documented problem?

Add callsign now that LNT is in phabricator

In D94840#2503260, @tnfchris wrote:

was it being indirectly imported before?

LGTM assuming tox is clean.

Rename variable in pairs() to avoid noqa hint

In D94791#2501415, @tnfchris wrote:

This seems rather random compared with D94789... so the spacing rule isn't consistent for assignments?

LGTM

LGTM

Landed as 80ed0db735b4636d40fb780605b9f947a19f7b7c

In D94418#2500546, @danilaml wrote:

I'm personally more enclined to prevent lit from running without lit installed because then some of the tests are skipped.

Do you mean without lnt installed? If so, I agree. It makes sense to expect LNT to b fully configured/installed before running the tests.
I think the initial test skip was done in case cPerf fails to build: https://github.com/llvm/llvm-lnt/commit/a57bf295f7988de9d9f5fbf30b429cf03d54271d
Not sure if it's still relevant for supported platforms (does it build on Mac?). Even if it doesn't, no harm in having those tests failed there.

Not sure about PYTHONPATH manipulations. Go ahead, if it fixes running under tox.

I see you've already fixed the issue with LNT :: server/ui/V4Pages.py I had.
Is the buildbot still running for LNT? I feel uneasy seeing how many tests were silently failing.

Rebase on top of D94715

Explain reason for noqa inline

Landed as 0d067a27868dae963ac9bc4a21e7da434822dd01

Landed as 52ea66719d01d3d18651946c1b83790ca371d34c

In D94418#2498777, @thopre wrote:

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

%t.err has:

2021-01-14 18:07:42 WARNING: Traceback (most recent call last):

File "/home/thomasp/repos/lnt/lnt/testing/profile/perf.py", line 32, in deserialize
  data = cPerf.importPerf(f, nm, objdump)

NameError: global name 'cPerf' is not defined

I'm not sure why cPerf appears not to be installed under tox, it should build everything. tests/testing/cPerf.py does not have this problem because it skips the test if cPerf is not found.

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

In D94418#2498419, @danilaml wrote:

In D94418#2498383, @thopre wrote:

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

Do you use tox to run the tests?

I use lit directly (lit -svv ./tests) which what I understand tox does as well. I've rerun all tests using tox and the only test that failed was server/ui/V4Pages.py (I believe it's because I've already had an lnt instance running and that test doesn't account for that possibility).

In D94418#2498376, @danilaml wrote:

Sorry, pushed to the wrong branch (Phabricator/arcanist workflow is annoying >_<) and didn't notice your comment.

Hm, I've made sure that all tests pass on both py2 and py3 before submitting and I can't reproduce it on my machine.
Could you check if test_suite-profile-import.py.tmp.err and test_suite-profile-import.py.tmp.log have any suspicious errors (they should be in <..>/test_run_tmp/runtest/Output/)?

In D94418#2498190, @thopre wrote:

I've tried applying your patch and its test fail for me.

I've tried applying your patch and its test fail for me.

I've seen it landed on the upstream branch rather than the main branch. Was that a mistake?