Add support for '-explicit-dependency-graph-format=' flag #1181
Conversation
|
@swift-ci test |
artemcm
force-pushed
the
EmitInterModRealG
branch
from
12993f5 to
3825722
Compare
|
@swift-ci test |
| diagnosticEngine.emit(Error.optionRequiresAnother(Option.printExplicitDependencyGraph.spelling, | ||
| Option.driverExplicitModuleBuild.spelling)) | ||
| } | ||
| // '-explicit-dependency-graph-format=' requires '-print-explicit-dependency-graph' |
There was a problem hiding this comment.
Could -print-explicit-dependency-graph just take a format, or would that break all the things? (ie. do we need to be backwards compatible?)
There was a problem hiding this comment.
We don't need it to be backwards compatible, I don't know of anyone who uses this other than me but the ergonomics of this seemed much more obvious by not conflating two things: emit graph, and emit format.
There was a problem hiding this comment.
Is it conflating two things 🤔? If we're emitting something we're emitting it in some format, so that seems relevant to me. I don't particularly mind here, I just personally prefer the one.
There was a problem hiding this comment.
Gah, sorry I missed this comment.
That's fair, let me add to my previous comment: my intent was to separate the common case of wanting to emit this graph (just dump the complete JSON) from having to specify a more-specific output format. And I don't know of a way to have a separate or joined flag that optionally takes a parameter is there one?
There was a problem hiding this comment.
I don't think there is. My point was more I think the format is always relevant and thus I'd be fine with specifying json all the time for the "default" case. But I can see the pros and cons, so it doesn't really matter :).
| public static let embedTbdForModule: Option = Option("-embed-tbd-for-module", .separate, attributes: [.frontend], helpText: "Embed symbols from the module in the emitted tbd file") | ||
| public static let emitAbiDescriptorPath: Option = Option("-emit-abi-descriptor-path", .separate, attributes: [.frontend, .noDriver], metaVar: "<path>", helpText: "Output the ABI descriptor of current module to <path>") | ||
| public static let emitAssembly: Option = Option("-emit-assembly", .flag, attributes: [.frontend, .noInteractive, .doesNotAffectIncrementalBuild], helpText: "Emit assembly file(s) (-S)", group: .modes) | ||
| public static let emitAst: Option = Option("-emit-ast", .flag, alias: Option.dumpAst, attributes: [.frontend, .noInteractive, .doesNotAffectIncrementalBuild]) |
There was a problem hiding this comment.
Crazy how many options we add. Didn't I only just run this? This also means that these were only added for the old driver :(
There was a problem hiding this comment.
My observation is that most of these are .frontend flags, so at least they do not need the logic modified here.
| let swiftModuleInterfacesPath: AbsolutePath = testInputsPath.appending(component: "ExplicitModuleBuilds").appending(component: "Swift") | ||
| let sdkArgumentsForTesting = (try? Driver.sdkArgumentsForTesting()) ?? [] | ||
|
|
||
| let baseCommandLine = ["swiftc", |
There was a problem hiding this comment.
Not particularly relevant for this review, but IMO it'd be really nice if we could go from typed argument -> string. Ie. instead of -explicit-module-build just .explicitModuleBuild.str (or something similar). Though I suppose there's an argument to be made for making renaming arguments fairly difficult (ie. would have to rename in tests) so that it's not done accidentally 🤷
| // Restore the error stream to what it was | ||
| TSCBasic.stderrStream = errorStream | ||
| } | ||
| internal func withHijackedBufferedErrorStream( |
There was a problem hiding this comment.
Is there a reason we write to files here rather than just passing back a string? Also, could we pass the stream instead? eg.
@discardableResult
func withHijacked(stream: ThreadSafeOutputByteStream, body: () throws -> Void) rethrows -> String { ... }
Or if the body needs the stream, pass in the stream instead. Though from a quick look at uses, that argument is either ignored or used in a bunch of asserts at the end of the body (which IMO should be out of the body).
Up to you whether you think that's all worth cleaning up.
There was a problem hiding this comment.
I've cleaned this up to return a String instead, that's much cleaner indeed, thank you!
I'll leave the two streams as separate for now, I ran into some trouble when trying to pass in a stream that I will look into later.
There was a problem hiding this comment.
Nice :). It might be better as separate anyway, ie. I don't have to know the stream name this way 🤷
artemcm
force-pushed
the
EmitInterModRealG
branch
from
3825722 to
64ec062
Compare
|
@swift-ci test |
2 participants
This flag allows printing of the inter-module dependency graph using either JSON (default) or DOT formats.
Added in swiftlang/swift#61056.