[PATCH] D81527: [llvm-install-name-tool] Add `delete_rpath` option

[Sameer Arora via Phabricator via llvm-commits]

sameerarora101 marked an inline comment as done.
sameerarora101 added inline comments.


================
Comment at: llvm/test/tools/llvm-objcopy/MachO/install-name-tool-delete-rpath.test:13
+## Deleting single RPath entry:
+# RUN: llvm-install-name-tool -delete_rpath @executable_a/. %t
+# RUN: llvm-objdump -p %t | \
----------------
sameerarora101 wrote:
> jhenderson wrote:
> > sameerarora101 wrote:
> > > jhenderson wrote:
> > > > It's generally more traditional for llvm-objcopy tests to use a separate output file rather than modifying in place. This means you don't have to repeatedly recreate the same input in later tests. In some cases, we even demonstrate that the input file hasn't been modified, though I'm not convinced by the necessity of that here.
> > > > 
> > > > Same applies for all test cases in this test.
> > > Unlike `llvm-objcopy`, it seems like `install-name-tool` doesn't have an option (like `-o`) where the user can specify the output binary. It only expects an input file and thus can only change the binary in place. (It also doesn't print the result on the terminal when it succeeds so `>` operator can't be used)
> > `llvm-objcopy` (unlike `llvm-strip`) doesn't have a `-o` option either. It uses the following syntax:
> > 
> > ```
> > llvm-objcopy <input> [output]
> > ```
> > 
> > In other words, the second positional argument is the output file the input file will be copied to. If not specified, the input will be modified in place.
> > 
> > According to the help text, `llvm-install-name-tool` uses the same syntax, although I don't know if that's the case for Apple's version. Either way, assuming the help text is accurate, I think we should follow the same approach.
> Oh, it seems like the help text is wrong in case of  `llvm-install-name-tool`. If I run something like `./bin/llvm-install-name-tool -add_rpath @ZZ Dependent Dep2`, it throws the following error: `error: llvm-install-name-tool expects a single input file`. 
> 
> the code too sets `Config.InputFilename` and `Config.OutputFilename` to the same file:
> ```
>   if (Positional.size() > 1)
>     return createStringError(
>         errc::invalid_argument,
>         "llvm-install-name-tool expects a single input file");
>   Config.InputFilename = Positional[0];
>   Config.OutputFilename = Positional[0];
> ```
> Nevertheless, the current llvm's version is compatible with Apple's version as it too throws an error when an output file is specified. I can create a patch for the help text in a separate diff? (or, if preferable, make `install-name-tool` accept a output file?)
currently resolved by updating the help text here https://reviews.llvm.org/D81907. (I can change it to accept a output file if people prefer that way?) Please lemme know.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81527