New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SR-14122] Improve usability of -l flag. #56503
Comments
@swift-ci create |
If you would like to tackle this bug, feel free to ask questions here or tag me on GitHub (@ varungandhi-apple) to review your PR. |
Comment by Shreya Sharma (JIRA) Hello, |
Sure. [More generally, if something is marked as a StarterBug and there is no discussion and no one has assigned it to themselves, you should feel free to assign it to yourself.] The first thing that needs to be done here is you need to track down where the flag is defined in Testing wise, we have some tests under |
Comment by Shreya Sharma (JIRA) I'm still stuck on this one. I'm facing a problem in understanding the errors. In `/include/swift/Option/Options.td` I've tried defining an alias, or using JoinedOrSeparate and the error led me to `test/Driver/options-repl.swift` but no success so far. |
Turns out, I underestimated the complexity, sorry about that. (Also, please feel free to ask for help sooner so that you don't spend a lot of time spinning your wheels.) Hmm, it looks like changing the parsing changes the output as well. You must've seen this in the FileCheck output: /Users/varun/foss-swift/swift/test/Driver/options-repl.swift:32:19: error: LLDB-OPTS-DAG: expected strin
g not found in input
// LLDB-OPTS-DAG: -lsomelib
^
<stdin>:1:58: note: scanning from here
/Library/Developer/CommandLineTools/usr/bin/lldb "--repl=-target x86_64-apple-macosx10.9 -enable-objc-in
terop -sdk / -I \"this folder\" -F /path/to/frameworks -module-cache-path BUILD_DIR/swift-test-results/x
86_64-apple-macosx10.9/clang-module-cache -swift-version 4 -D A -D B -D C -D D -ignore-module-source-inf
o -L /path/to/libraries -L /path/to/more/libraries -l somelib -framework SomeFramework"
^ We need to make sure that we don't pass in the space to LLDB in case it breaks. If you look at the test, there is the context.Args.AddAllArgs(FrontendArgs, options::OPT_l, options::OPT_framework,
options::OPT_L); I'm guessing that the template<typename ...OptSpecifiers>
void AddLastArg(ArgStringList &Output, OptSpecifiers ...Ids) const {
if (Arg *A = getLastArg(Ids...)) // Calls claim() on all Ids's Args.
A->render(*this, Output);
} The I tried searching for how RenderStyleKind getRenderStyle() const {
if (Info->Flags & RenderJoined)
return RenderJoinedStyle;
if (Info->Flags & RenderSeparate)
return RenderSeparateStyle;
switch (getKind()) {
case GroupClass:
case InputClass:
case UnknownClass:
return RenderValuesStyle;
case JoinedClass: // <-- before
case JoinedAndSeparateClass:
return RenderJoinedStyle;
case CommaJoinedClass:
return RenderCommaJoinedStyle;
case FlagClass:
case ValuesClass:
case SeparateClass:
case MultiArgClass:
case JoinedOrSeparateClass: // <-- now
case RemainingArgsClass:
case RemainingArgsJoinedClass:
return RenderSeparateStyle; // <--
}
llvm_unreachable("Unexpected kind!");
} I suspect that's why it is getting rendered as separate, whereas earlier we had it be We could manually do some string concatenation to make sure we render I'm trying to see if there is a better way where we specify how the argument can be rendered in one place, and make sure |
Hmm, there are only 2 places which are relying on this. So I think you can add a new helper function, say ToolChain::InvocationInfo
ToolChain::constructInvocation(const InterpretJobAction &job,
const JobContext &context)
ToolChain::InvocationInfo
ToolChain::constructInvocation(const REPLJobAction &job,
const JobContext &context) const |
Comment by Shreya Sharma (JIRA) Thank you so much for the detailed explanation. I can see why `JoinedOrSeparate` fails. I tried `JoinedAndSeparate` here to see the result but ended up with errors. Can you please explain how `JoinedAndSeparate` works? |
What -myflagmyarg1 myarg2 And -larg1 arg2 We would like the Given that, I think the right fix here is to continue with
Hope that helps! |
Comment by Rajagopalan Gangadharan (JIRA) Hello theindigamer (JIRA User) , is this ticket still available for grabs? If yes, could I work on this 🙂 |
Sure, go for it. |
Comment by Rajagopalan Gangadharan (JIRA) theindigamer (JIRA User) Ping |
You're on Linux, so there are a few differences between platforms.
For Linux, I would try a variation of (I haven't tested this but I think it ought to work, maybe with a tweak or two). 'public func f() { print("Hello") }' > tmp1.swift
swiftc tmp1.swift -emit-module-path tmp1.swiftmodule -emit-library -o libtmp1.a
echo 'import tmp1\n\ntmp1.f()' > tmp2.swift
swiftc tmp2.swift -I . -c -o tmp2.o
swiftc tmp2.o -L . -ltmp1 Wrt pings, I appreciate that you called my attention to it, but I'm generally not checking my work email or attending to Jira over the weekend. |
Comment by Rajagopalan Gangadharan (JIRA) Oh ya, I feel dumb should have realized this sooner. Thank you so much for this detailed information. And regarding the ping - sorry will keep this in my mind from next time 🙂 |
Comment by Rajagopalan Gangadharan (JIRA) theindigamer (JIRA User) please review this PR - #38750 |
Attachment: Download
Additional Detail from JIRA
md5: 5811a36dd197d1a76e8beeedde7c7974
Issue Description:
According to the swiftc help page, one can use
-l<value>
to ask it to link to a library. However, if one adds a space-l <value>
, then swiftc emits a strange errorerror: no such file or directory: 'tmp1'
.We should be consistent with
-L
and allow a space between the flag and the argument. Alternately, if we want to be consistent withld64
, we can emit an error instead that we got an empty value for-l
("did you add an extra space after '-l'?").Here's an example of when one could hit this error:
Fixing this to
-ltmp1
would fix the issue (as you can check by running./tmp2
).The text was updated successfully, but these errors were encountered: