Build: avoid unnecessary warnings for build tool inputs #7300
Conversation
…deration and don't emit unnecessary warnings
pusukuri
requested review from
neonichu,
tomerd and
MaxDesiatov
as code owners
|
@swift-ci test |
MaxDesiatov
changed the title
MaxDesiatov
changed the title
|
@swift-ci test macos |
MaxDesiatov
added
bug
plugins
build system
|
@swift-ci test macos |
There was a problem hiding this comment.
I think we need to expand the test case here to include a deliberately unhandled resource that we should continue to warn about, e.g. Fixtures/Miscellaneous/Plugins/MySourceGenPluginNoPreBuildCommands/Tests/MySourceGenPluginNoPreBuildCommandsTests/lunch.txt.
I'm seeing inconsistent behavior if I do this and I think that's related to the fact that if we are planning for plugins, we'll use a subset, but if we don't need to do that, we don't. We need to figure out how to solve that, likely this will involve not emitting the diagnostics as part of planning but at some other stage or it would involve changing how we do the build for executables needed for plugins.
|
@swift-ci test |
|
@neonichu: I have addressed your comment. Please take a look. Thank you. |
|
@swift-ci test windows |
MaxDesiatov
changed the title
There was a problem hiding this comment.
How exactly does this fixture differ from other similar fixtures? I find that Sample source generator tool in main.swift has been copied over at least a few times in Fixtures/Miscellaneous/Plugins. Can we reduce that duplication? If not, I would love to see a README.md added to Fixtures/Miscellaneous/Plugins directory that describes available fixtures related to plugins and difference between them.
| @@ -34,6 +34,15 @@ class PluginTests: XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func testUseOfBuildToolPluginTargetNoPreBuildCommandsByExecutableInSamePackage() throws { | |||
There was a problem hiding this comment.
This name is too long and difficult to read. Would we be able to come up with a more concise name that still uniquely identifies the test?
Sources/Build/BuildOperation.swift
Outdated
| diagnosticsEmitter.emit(warning: warning) | ||
| // rdar://113256834 This fix works for the plugins that do not have PreBuildCommands. | ||
| let targetsToConsider: [ResolvedTarget] | ||
| if let subset = subset, let recursiveDependencies = try subset.recursiveDependencies(for: graph, observabilityScope: observabilityScope) { |
There was a problem hiding this comment.
Line length nit. In the future, you can run ./Utilities/soundness.sh in the root of a SwiftPM clone before committing to fix these formatting issues automatically, which may require some final manual vetting and tweaking after that.
| if let subset = subset, let recursiveDependencies = try subset.recursiveDependencies(for: graph, observabilityScope: observabilityScope) { | |
| if let subset = subset, let recursiveDependencies = try subset.recursiveDependencies( | |
| for: graph, | |
| observabilityScope: observabilityScope | |
| ) { |
There was a problem hiding this comment.
Done @MaxDesiatov Thank you
|
@swift-ci test |
| metadata.targetName = target.name | ||
| return metadata | ||
| } | ||
| var warning = "found \(unhandledFiles.count) file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n" |
There was a problem hiding this comment.
Would a multiline string work here? It would also make line length warnings for formatters and linters go away.
| var warning = "found \(unhandledFiles.count) file(s) which are unhandled; explicitly declare them as resources or exclude from the target\n" | |
| var warning = """ | |
| found \(unhandledFiles.count) file(s) which are unhandled; explicitly declare them \ | |
| as resources or exclude from the target\n | |
| """ |
| @@ -782,6 +792,27 @@ extension BuildDescription { | |||
| } | |||
|
|
|||
| extension BuildSubset { | |||
| func recursiveDependencies(for graph: PackageGraph, observabilityScope: ObservabilityScope) throws -> [ResolvedTarget]? { | |||
There was a problem hiding this comment.
| func recursiveDependencies(for graph: PackageGraph, observabilityScope: ObservabilityScope) throws -> [ResolvedTarget]? { | |
| func recursiveDependencies( | |
| for graph: PackageGraph, | |
| observabilityScope: ObservabilityScope | |
| ) throws -> [ResolvedTarget]? { |
| @@ -34,6 +34,15 @@ class PluginTests: XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func testUseOfBuildToolPluginTargetNoPreBuildCommands() throws { | |||
There was a problem hiding this comment.
| func testUseOfBuildToolPluginTargetNoPreBuildCommands() throws { | |
| func testBuildToolWithoutPreBuildCommands() throws { |
| @@ -34,6 +34,15 @@ class PluginTests: XCTestCase { | |||
| } | |||
| } | |||
|
|
|||
| func testUseOfBuildToolPluginTargetNoPreBuildCommands() throws { | |||
| // Only run the test if the environment in which we're running actually supports Swift concurrency (which the plugin APIs require). | |||
There was a problem hiding this comment.
| // Only run the test if the environment in which we're running actually supports Swift concurrency (which the plugin APIs require). | |
| // Only run the test if the environment in which we're running actually supports Swift concurrency (which | |
| // the plugin APIs require). |
|
@swift-ci test windows |
Consider plugin build command input files into consideration and don't emit unnecessary warnings
Motivation:
Starting with 5.9 SwiftPM produces warnings for unhandled files but doesn’t take plugin build command input files into consideration. Consider plugin build command input files into consideration and don't emit unnecessary warnings.
Resolves rdar://113256834