Assert that only declared dependencies are used when compiling/linking #466
Conversation
| @@ -50,7 +50,7 @@ fileprivate extension CommandResult { | |||
| } | |||
|
|
|||
| extension TaskAction { | |||
| func spawn(commandLine: [String], environment: [String: String], workingDirectory: String, dynamicExecutionDelegate: any DynamicTaskExecutionDelegate, clientDelegate: any TaskExecutionClientDelegate, processDelegate: any ProcessDelegate) async throws { | |||
| static func spawn(commandLine: [String], environment: [String: String], workingDirectory: String, dynamicExecutionDelegate: any DynamicTaskExecutionDelegate, clientDelegate: any TaskExecutionClientDelegate, processDelegate: any ProcessDelegate) async throws { | |||
There was a problem hiding this comment.
As it stands, I need to use this from the introduced TaskDependencyVerification.Adapter.exec() (volatile link), so made it static. This doesn't feel right.
A better home might be as an extension of the newly introduced TaskExecutionContext (volatile link). That's an even more invasive change though.
Any suggestions on how to clean this up welcome.
There was a problem hiding this comment.
Moving to TaskExecutionContext seems fine; there's no particularly strong reason I put this here.
Sources/SWBTaskExecution/TaskActions/ClangCompileTaskAction.swift
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| func findLibraryName() -> String? { | ||
| if fileExtension == "a" && basename.starts(with: "lib") { |
There was a problem hiding this comment.
Libraries on Windows don't start with lib. This is also not technically required on Unix either, so we should think how to handle mapping of libraries that aren't necessarily using the lib prefix.
There was a problem hiding this comment.
I've more deeply encapsulated this and added clarifying documentation that it does not attempt to be a viable implementation for general use.
The intention is not to do more with this at this time in this change set, but evolve later.
How do you suggest we resolve the partial status of this for this PR?
ldaley
force-pushed
the
ldaley/dependency-verification
branch
from
570e1c2 to
7699bd5
Compare
|
@swift-ci test |
|
@mirza-garibovic @jakepetroules this is ready for another review pass. I don't expect that it is mergeable, but there is nothing that I intend to add change other than addressing feedback-to-come. |
ldaley
changed the title
ldaley
requested review from
aciidgh,
mirza-garibovic,
mhrawdon,
neonichu and
owenv
as code owners
…-time This is a skeletal implementation scoped to support a specific functional milestone.
Also adds disclaimer that implementation is provisional.
ldaley
force-pushed
the
ldaley/dependency-verification
branch
from
0cd961d to
73e0bd7
Compare
|
@swift-ci test |
This is the first increment towards supporting centralized declared dependencies.
This introduces a
DEPENDENCIESbuild setting, that is a string list of “logical” dependency names (e.g.FooforFoo.framework). When invoking a tool that supports trace information, the trace will be cross referenced with the declared dependencies and the action will fail if a file is used that does not “resolve” to a logical dependency declared inDEPENDENCIES. This process of analyzing trace data and asserting that only declared dependencies are used is referred to as “verification”.This PR adds verification for linking, and non-modular clang compilation. Later changes will add support for other trace-producing tools.
The error reporting and diagnostic experience in this change is considered rudimentary/provisional, and will be improved in subsequent changes.