[PATCH] D98859: [lit] Handle plain negations directly in the internal shell

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In D98859#2669167 <https://reviews.llvm.org/D98859#2669167>, @jdenny wrote:

> With the change you proposed, what happens to `not --crash not cmd`?  Won't the inner `not` be caught as a fake external?  I suppose the `not --crash` would fail either way, but stderr would be different, and it would be more confusing to debug.

No, that case would work. My fake `not` executable looks like this:

  #!/usr/bin/env python
   
  import fake_external
  import os
  import subprocess
  import sys
   
   
  if len(sys.argv) > 1 and sys.argv[1] == '--crash':
    # Call the real 'not' binary; remove the dirname of argv[0] from the PATH
    paths = os.environ['PATH'].split(os.pathsep)
    cur_dir = os.path.dirname(sys.argv[0])
    if cur_dir in paths:
      paths.remove(cur_dir)
    os.environ['PATH'] = os.pathsep.join(paths)
    # Call the other 'not', replace argv[0] with a 'not' without a potential
    # path, to let it search for it in the path.
    args = sys.argv
    args[0] = 'not'
    sys.exit(subprocess.call(args))
   
  fake_external.execute(__file__)

So the fake-externals dir gets removed from the path when re-executing `not --crash`.

> It seems to me this is getting too confusing.  Sorry, my fake external suggestion is probably not worth the effort right now.  Thanks for looking into it.
>
> If you agree with that assessment, would you extend the `fake-externals` comments in `llvm/utils/lit/tests/lit.cfg` to mention why `not` isn't included?  You might say: Don't do this for 'not' because lit uses the external 'not' throughout a RUN line that calls 'not --crash'.

Sure, that sounds good to me.

> Eventually, it would be great to see all this handling simplified.  It seems one way to do that is to implement `not --crash` in python.  But that's a different patch.

Yup, that sounds like a more worthwhile future direction.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98859