This is an archive of the discontinued LLVM Phabricator instance.

[llvm-libtool-darwin] Add support for -filelist option
ClosedPublic

Authored by sameerarora101 on Jul 20 2020, 3:57 PM.

Details

Summary

Add support for -filelist option for llvm-libtool-darwin
-filelist option allows for passing in file containing a list of filenames.
Depends on D83520.

Diff Detail

Event Timeline

sameerarora101 created this revision.Jul 20 2020, 3:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 20 2020, 3:57 PM

Documentation for the new option?

llvm/test/tools/llvm-libtool-darwin/filelist.test
29

Why are you trying to delete some files here?

33

I'm not sure this comment is accurate for what the error is indicating. It needs to provide more context.

67

This seems like essentially a duplicate of the earlier error case?

95

Additional possible test-cases:

  1. Multiple file lists specified.
  2. File is in working directory, but a directory name is specified.
  3. Directory exists but does not contain the requested file.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
47

Nit: your -o option doesn't include a trailing full stop in its description. Be consistent.

By the way, the pre-merge bot is saying your new test is failing on Windows.

sameerarora101 marked 8 inline comments as done.Jul 21 2020, 9:02 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/filelist.test
29

I just wanted to make sure that %basename_t.tmp-input1.o didn't exist in the current directory. However, I have now removed the error test just below (and therefore removed RUN: rm -f %t-input1.o && rm -f %basename_t.tmp-input1.o) as it was similar to the "file not in filelist test" as you highlighted. Thanks!

67

yup, you are right. I have now removed the previous test case

95

3rd case is already there:

## Check that error is thrown when file in filelist doesn't exist (with dir):
# RUN: rm -rf %t/Invalid-Dir && mkdir -p %t/Invalid-Dir
# RUN: echo 'no-such-file' > %t.invalid-list.txt
# RUN: not llvm-libtool-darwin -static -o %t.lib -filelist %t.invalid-list.txt,%t/Invalid-Dir 2>&1 | \
# RUN:   FileCheck %s --check-prefix=FILE-ERROR -DFILE=%t/Invalid-Dir/no-such-file

I have modified the comment to make it more clear. Added the first 2 cases now. Thanks.

sameerarora101 marked 3 inline comments as done.Jul 21 2020, 9:02 AM

Add documentation and updating test file

Unsupport a test on windows and thow blank-line error earlier.

sameerarora101 marked an inline comment as done.Jul 21 2020, 10:38 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
69–70

I was getting an assertion error that filenames can't be empty. Hence, now I am explicitly throwing the error here only.

MaskRay added inline comments.Jul 21 2020, 11:14 PM
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
61

line_iterator supports is_at_eof. You can delete E

187

Prefer return to exit in main

jhenderson added inline comments.Jul 22 2020, 6:03 AM
llvm/docs/CommandGuide/llvm-libtool-darwin.rst
52

I'd suggest "Whitespace on a line is assumed..."

llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test
9 ↗(On Diff #279573)

Could you just use echo '' >> %t.blank-line.txt to get your blank line, so that this can run on Windows?

15 ↗(On Diff #279573)

Similarly, here, use echo -n " " here. Why the -n at all in fact in this case?

17 ↗(On Diff #279573)

I'd suggest using FileCheck --strict-whitespace in this case to be able to match the filename properly.

llvm/test/tools/llvm-libtool-darwin/filelist.test
60

error -> an error
file -> a file
filelist -> the filelist
cwd -> the cwd

67

an error
the directory

68

I'd recommend a different name for the directory. The directory itself is not invalid, so calling it that is a little confusing. How about just reusing the directory from before?

75

an error
a file
the cwd

95

Thanks. What about one for the directory not existing at all?

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
69–70

I'd be tempted to use a different error message to show that this error isn't a system error. Perhaps simply "filename cannot be empty".

sameerarora101 marked 15 inline comments as done.Jul 22 2020, 8:12 AM
sameerarora101 added inline comments.
llvm/test/tools/llvm-libtool-darwin/filelist-error-blank-lines.test
9 ↗(On Diff #279573)

oh nice, thanks!

15 ↗(On Diff #279573)

Yup, works without -n.

llvm/test/tools/llvm-libtool-darwin/filelist.test
95

Added now.

sameerarora101 marked 3 inline comments as done.Jul 22 2020, 8:22 AM
  • Supporting test on windows
  • Using is_at_eof for line_iterator
jhenderson accepted this revision.Jul 23 2020, 12:33 AM

LGTM, with nit, thanks!

llvm/test/tools/llvm-libtool-darwin/filelist.test
95

Let's check the full error line. Presumably, this at least has error: file list file: or something similar before it?

This revision is now accepted and ready to land.Jul 23 2020, 12:33 AM
sameerarora101 marked an inline comment as done.Jul 23 2020, 7:30 AM

Updating the error message.

smeenai accepted this revision.Aug 4 2020, 2:59 PM

LGTM

llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
78

Nit: SmallString has an operator std::string, so you should be able to convert directly without the intermediate Twine. I'm not sure if just InputFiles.push_back(Path) will compile, but a cast should definitely trigger the conversion, e.g. InputFiles.push_back(static_cast<std::string>(Path)).

sameerarora101 marked an inline comment as done.Aug 5 2020, 7:44 AM
sameerarora101 added inline comments.
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp
78

Yup, casting works. Thanks.

sameerarora101 marked an inline comment as done.

Twine(Path).str() -> static_cast<std::string>(Path)