[PATCH] D54884: [bugpoint] Find 'opt', etc., in bugpoint directory

[Brian Gesiak via Phabricator via llvm-commits]

modocache marked an inline comment as done.
modocache added a comment.

Thanks @MatzeB! I'll change this to search in the bugpoint directory first, and fall back to the PATH.

I also responded to your comments on the test. As I mention in the comment there, I think that once I change the search behavior and confirm all tests still pass, this patch seems good enough to commit. Let me know if you disagree!



================
Comment at: test/BugPoint/compile-custom.ll:1
-; RUN: bugpoint -load %llvmshlibdir/BugpointPasses%shlibext --compile-custom --compile-command="%/s.py arg1 arg2" --opt-command opt --output-prefix %t %s | FileCheck %s
+; RUN: bugpoint -load %llvmshlibdir/BugpointPasses%shlibext --compile-custom --compile-command="%/s.py arg1 arg2" --output-prefix %t %s | FileCheck %s
 ; REQUIRES: loadable_module
----------------
MatzeB wrote:
> Won't this pick up an `opt` in the PATH now if one is available? That would make the test unstable.
> 
> Similar in the other tests.
Oh, I see. I didn't realize that LLVM's test/lit.cfg.py provides a //substitution// named `opt` -- so this actually expands to the lit command `--opt-command /path/to/build/bin/opt`:

```
# llvm/test/lit.cfg.py

tools.extend([
    # ...
    'verify-uselistorder', 'bugpoint', 'llc', 'llvm-symbolizer', 'opt',
    'sancov', 'sanstats'])

# ...

llvm_config.add_tool_substitutions(tools, config.llvm_tools_dir)
```

However, I believe the tests are still stable after this change, and here's why: test/lit.cfg.py also inserts the built LLVM tools directory at the beginning of the PATH when running tests:

```
# Tweak the PATH to include the tools dir.
llvm_config.with_environment('PATH', config.llvm_tools_dir, append_path=True)
```

So during the test run, `/path/to/build/bin` is always the first item in the PATH. Since the default behavior of `bugpoint` is to try and find `opt` in the PATH when no `--opt-command` is specified, removing this option has no material impact on the tests.

So this means two things:

1. The usages of `--opt-command` in the bugpoint tests could all be removed today, even without this patch, as long as lit.cfg.py continues to prepend the tools directory to the PATH when running tests.
2. Since these options can be removed even without this patch, removing them isn't a sufficient way to test the behavior in this diff. Still, I think removing the usage of the options, and changing the behavior of bugpoint so it searches in the bugpoint executable's directory first, might be a "good enough" test of the new behavior. The only alternative I can think of is having a test that copies the bugpoint executable into a temporary directory, copying a test programmed named "opt" into that same directory, and invoking bugpoint in some way that demonstrates the "opt" from the temporary directory is chosen... and that seems like overkill to test this minor convenience.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54884