Add support for passing in libraries via -l and -L options. Depends
on D85334.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
2 | It doesn't make sense to test -L option individually as the directory specified under that option is used (prepended) only if some library is also specified with option -l. To test -l individually, I would need to create a library in one of the standard directories (/lib/, /usr/lib, /usr/local/lib). Is there a way I can do that in a test file? Thanks. | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
84–88 | I defined StandarDirs based on cctools' libtool: /* the standard directories to search for -lx names */ char *standard_dirs[] = { "/lib/", "/usr/lib/", "/usr/local/lib/", NULL }; |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
2 | I don't think so. Other tools have options (like --sysroot) to re-root the search, but it doesn't appear that libtool has anything like that, so I don't think we'll be able to test that case. | |
8 | I'd prefer for a file ending in .o to actually be an object file and not an archive :) | |
86 | Other test ideas:
| |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
101 | Any reason for creating this variable? SmallString can be implicitly converted to StringRef instead, and it can also be converted to std::string explicitly (e.g. via std::string(Path)). | |
103 | Nit: you don't need the braces | |
110 | The duplication between the two loops is unfortunate, but I don't think the body of the loops is large enough to justify moving it out into a function (especially if you get rid of the PathString variable). What we really want is a C++ equivalent to Python's itertools.chain, and I think we might get that with ranges in C++20. | |
125 | One of the following should work, I think. StringRef BaseName = s; StringRef BaseName(s); | |
129 | I would construct a Twine and have searchForFile take const Twine & instead. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
8 | I just wanted to check that a library ending in .o is not prepended/appended with some additional string lol. How about the name archive-object.o? | |
86 | Thanks, added now. For the second test case, I assumed you meant passing in a library with -l along with a normal input file? Or did you mean prepending -l to an input file name? For the third case, libtool throws a warning (similar to what it does when we specify the same input file twice). I have added a comment about that in my test case above now. | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
101 | Ah forgot about the explicit conversion, thanks! | |
125 | Both work 😄 | |
129 | Seems like I can't construct a Twine before the if condition and modify it appropriately in the corresponding branches (which is needed so as to have the Twine variable in scope after the if condition). Furthermore, I also cannot directly call searchForFile(<Twine>); within each branch and assign the result to Expected<std::string> FullPath, as I cannot declare Expected<std::string> FullPath; before the if branch. Sorry if I didn't get what you were trying to explain here ? |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
8 | I think the intent is, even though the option is -l (with the l presumably standing for library), you can still use it to specify input object files (so that you don't have to specify their full path). What I meant was to just have an object file and use -l to add that, instead of creating an archive and giving it a .o extension. | |
86 | Yup, that's what I meant for the second case. | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
129 | Hmm, good points. Lemme think if there's anything else we can do here. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
8 | Ah I see. Ok, I'll update it. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
8 | You should place lines like this that are tied to an individual test-case, below the comment that goes with the case, as that explains the purpose of the line. | |
27 | ||
117 | I might suggest changing the -l file's name here to something even more arbitrary such as "-lfile-will-not-exist". Since libtool searches certain directories by default, there's a (small) chance the file being searched for is in one of those directories. The weirder we make the name, the smaller this chance, and therefore the less likely this test case will fail on somebody's system. | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
72–74 | I just want to make sure the behaviour here is definitely what's documented and that it doesn't search for libfoo.o.a for -lfoo.o? We probably need a test case for that. | |
97 | Could this be moved into the loop body, to avoid the Path.clear() line? | |
110 | Perhaps a lambda could work okay here: auto FindLib = [](ArrayRef<StringRef> SearchDirs) { for (StringRef Dir : SearchDirs) { SmallString<128> Path; sys::path::append(Path, Dir, FileName); if (sys::fs::exists(Path)) return std::string(Path); } return ""; }; std::string Found = FindLib(LibrarySearchDirs); if (Found.empty()) Found = FindLib(StandardSearchDirs); if (!Found.empty()) return Found; // report error | |
116 | ||
121 | Can't you just do for(StringRef BaseName : Libraries)? If not, at least do s -> S | |
129 | How about something like: Expected<std::string> FullPath = searchForFile(BaseName.endswith(".o") ? BaseName.str() : "lib" + BaseName + ".a"); |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
72–74 | Yup, the behavior here is definitely what's documented. My first test case is testing this only where I pass in -larchive-object.o. Also, I have now changed that test by passing in -l%basename_t.tmp-input1.o instead of archive-object as per @smeenai's recommendation above. | |
110 | I like the lambda too, updated now. Thanks! | |
116 | it is not trying to locate '%s' but might add lib and .a to the string and try locating lib%s.a, right? That's what I meant up there. I can change it to what you're suggesting if it doesn't make sense? | |
129 | works, thanks. |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
84 | Seems like I cannot have constexpr with std::array<std::string, 3>. It says, "constexpr variable cannot have non-literal type 'const std::array<std::string, 3>' (aka 'const array<basic_string<char>, 3>')". |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
72–74 | I think @jhenderson meant that we should also add a case where you have e.g. a file called libfoo.o.a, you pass -lfoo.o, and you make sure that you get an error about a missing file (i.e. confirming that when you have a -l argument ending in .o, you only search for the name as-is). |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
---|---|---|
72–74 | Oh, i see now. I'll add a test. Thanks for clarifying! |
llvm/docs/CommandGuide/llvm-libtool-darwin.rst | ||
---|---|---|
65 | Nit: remove the trailing slash on /lib/ | |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
101 | Can you test this for a name ending with .a as well? | |
103 | This one too? | |
112 | You need the -L%t/dirname as well, right? | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
84 | Ah, oh well. | |
106 | I'd prefer an Optional<std::string> instead of having the empty string represent the absence. | |
116 | FileName will have already had the lib and .a additions at this point though, right? | |
129 | Ah, I keep forgetting about ternary expressions. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
24 | Maybe worth naming archives with a single member in them after the member's name, e.g. here libinput2.a I think that helps readability, also possibly reusability (see below). | |
58–59 | Rather than create two new archives here, you could just create one, in otherDirname, containing input1.o. You already have a library in dirname containing input2.o, so if you name the new one the same, you can use that one too. | |
110 | Let's also add a test case with a file extension other than these two, e.g. .ext and show that the behaviour is specific to '.o'. | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
121 | It's not particularly idiomatic to put const with StirngRef. | |
129 | I avoid them in most cases, but this seems like a sensible usage. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
101 | Did you mean for a file to which '.a' is appended, right? (The name itself doesn't end in '.a' but since the name doesn't end in '.o', 'lib' and '.a' are added to it)? If that is what you meant, added a test for it now. Thanks. | |
112 | yup, added. thanks | |
llvm/tools/llvm-libtool-darwin/llvm-libtool-darwin.cpp | ||
116 | ah yes, you are right. Replaced as suggested. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
96–99 | One more test for this area: a file called something like file, and show that -lfile doesn't find it (since lib and .a are added). Relatedly, maybe also show that -llibfile.a looks for liblibfile.a not libfile.a? |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
96–99 | Added now, thanks. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
96–99 | This doesn't work if you do have a libfile.a, which is a real library that exists; on FreeBSD it comes as part of schilybase, and so having that package installed breaks this test. |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
96–99 | Heh, fixed in rG22b5fe74782a |
llvm/test/tools/llvm-libtool-darwin/L-and-l.test | ||
---|---|---|
96–99 | Great, thanks for the quick follow-up |
Nit: remove the trailing slash on /lib/