SR-12129: Diagnose lack of input files #72
Conversation
robinkunde
force-pushed
the
rk/SR-12129
branch
from
6c25c52 to
0cfda35
Compare
| case .invalidInput(let input): | ||
| return "invalid input: \(input)" | ||
| case .noJobsPassedToDriverFromEmptyInputFileList: | ||
| return "no input files were passed and no subsequent jobs were generated for the driver" |
There was a problem hiding this comment.
It might be best to truncate this to "no input files" for compatibility with the C++ driver
There was a problem hiding this comment.
Can do, but is there a way we can provide more informative error messages in the long term?
There was a problem hiding this comment.
Long term I think that's definitely a worthy goal. Right now, my concern is that we don't have a good way to catch regressions in the integration tests yet, and failures due to small differences like this can sometimes be hard to debug.
| @@ -604,7 +621,12 @@ extension Driver { | |||
| try printVersion(outputStream: &stderrStream) | |||
| } | |||
|
|
|||
| if jobs.isEmpty { return } | |||
| guard !jobs.isEmpty else { | |||
| guard !inputFiles.isEmpty else { | |||
There was a problem hiding this comment.
I think we should check this before trying to plan a build. It can probably go into Planning.swift's Driver.planBuild(), so long as the driver isn't creating a REPL or immediate forwarding job (which don't require inputs).
There was a problem hiding this comment.
In that case, we'd have to move optionTable.printHelp() and printVersion() out of Driver.run() as well, correct?
There was a problem hiding this comment.
You would. Help is handled by this: #66
Version is a bit more interesting, there is technically 3 version flags. -v, --version, and -version. -version and --version are treated the same, they print the version information and ignore everything else (Actually help takes higher precedence over version in the c++ compiler, which isn't the case in #66 @owenv). Now -v prints the version information, but then continues on, with the only difference being that it suppresses the no input files error. You can see that here: https://github.com/apple/swift/blob/master/lib/Driver/Driver.cpp#L2157. We can mimic that logic by adding a shouldSuppressNoInputFilesError Bool that gets set to the value of parsedOptions.hasArgument(.v).
So currently --version and -version are handled in planBuild, so -v should be too, but it needs to be treated differently, not as a immediateForwardingJob.
Here are some examples from the c++ driver, that I think show all the cases I mentioned above.
colton-mac:~ colton$ swiftc -typecheck
<unknown>:0: error: no input files
colton-mac:~ colton$ swiftc --version
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
colton-mac:~ colton$ swiftc --version -typecheck
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
colton-mac:~ colton$ swiftc -v -typecheck
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
colton-mac:~ colton$ swiftc -v
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
colton-mac:~ colton$ swift -version
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
colton-mac:~ colton$ swift -v
Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15)
Target: x86_64-apple-darwin18.7.0
/Applications/Xcode.app/Contents/Developer/usr/bin/lldb "--repl=-enable-objc-interop -sdk /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk -color-diagnostics"
Welcome to Apple Swift version 5.1.3 (swiftlang-1100.0.282.1 clang-1100.0.33.15).
Type :help for assistance.
1> :q
colton-mac:~ colton$
There was a problem hiding this comment.
yeah, sorry for the confusion, I forgot we hadn't come up with a good solution for -v yet, this is fine to leave as-is.
|
@cltnschlosser @owenv Hey folks, I was out sick last week and would like to move this forward now. Do I understand correctly that this is good to merge now, or should we merge #66 first and then I can make some changes to accommodate -v? |
|
@robinkunde IMO this is good to merge, someone with write access will need to approve first though. |
robinkunde
changed the title
|
@swift-ci test |
1 similar comment
|
@swift-ci test |
|
Thanks for fixing this @robinkunde , sorry for the delay in getting it merged! |
Merge pull request apple#72 from robinkunde/rk/SR-12129
Work in progress. I'd just like to see if I'm going in the right direction.