Fix build failures when two or more plugin-generated build commands have the same name #3585
Conversation
abertelrud
added
the
ready
|
@swift-ci please smoke test |
Sources/Build/ManifestBuilder.swift
Outdated
| @@ -586,8 +586,9 @@ extension LLBuildManifestBuilder { | |||
| for command in target.pluginInvocationResults.reduce([], { $0 + $1.buildCommands }) { | |||
| // Create a shell command to invoke the executable. We include the path of the executable as a dependency. | |||
| let execPath = AbsolutePath(command.configuration.executable, relativeTo: buildParameters.buildPath) | |||
| let uniquedName = ([command.configuration.displayName] + (command.inputFiles + command.outputFiles ).map{ $0.pathString }).joined(separator: "|") | |||
There was a problem hiding this comment.
would it make sense to use something like UUID to make sure this is always unique, or do you want this to match if inputs + outputs are the same?
There was a problem hiding this comment.
The main thing is to make sure that's stable, so that running the same inputs generates the same llbuild manifest. Otherwise we will have spurious rebuilds. The combination of inputs and outputs seems sufficient to make it unique — if it's the same inputs and outputs then from llbuild's perspective there is nothing to distinguish when they run.
I suppose we could include the command line in the hash as well — I'm not really sure what happens if you have two commands with the same inputs and outputs — would you expect both commands to run? Perhaps using the whole command line would be better.
There was a problem hiding this comment.
But UUIDs and things like that are usually the bane of build systems, since they introduce randomness, which generally defeats null builds, etc.
There was a problem hiding this comment.
In the general case, the question of whether a command that changes arguments from one run to the next is still the "same" command can be tricky. It can for example affect how things are shown in IDEs, but it's unlikely to matter from a correctness standpoint here. So even if, say, adding a "-v" flag from one run to the next doesn't change the identity of the command in a conceptual way, having it appear as a different command in the llbuild manifest might not cause any problems. I'm therefore inclined to hash the whole command line.
Perhaps the plugin identity and the target to which it was applied should factor in here, although it should be noted that they are already baked into the output paths, since every combination of plugin and target is given its own subdirectory under the intermediates directory.
There was a problem hiding this comment.
Not entirely sure what the plain llbuild behavior is, but XCBuild will error if there are multiple commands producing the same output file in a single build operation.
There was a problem hiding this comment.
In this case the llbuild manifest logic was discarding the older commands. In the case of a plugin that emits two commands with the same outputs, we should probably catch that earlier and emit an error. I'll write up an enhancement request for that.
|
Aha, we're already in the business of silently dropping commands on Linux: Perhaps that's a bug that we haven't detected before, or just a benign duplication. That warrants further investigation. Perhaps I should separate out those assertions into a separate PR from the plugin fix. |
|
Split out the assertions here: #3587 |
…y including the command line as well. Otherwise the llbuild manifest will contain only the last defined command with the same name, leading to build failures. To avoid having the name be exceedingly long, what we append to the name is a digest of the command line and not the full command line itself. rdar://79872414
abertelrud
force-pushed
the
eng/anders/79872414-plugin-commands-with-same-names-dont-work
branch
from
62bf7f7 to
5c3d5fa
Compare
|
@swift-ci please smoke test |
abertelrud
deleted the
eng/anders/79872414-plugin-commands-with-same-names-dont-work
branch
|
Seems I missed this -- but thanks a lot! :-) |
When two or more commands produced by plugins (either the same plugin or different plugins) had the same name, only one of them would run.
Motivation:
This is a critical build failure for plugins that don't incorporate some unique variable such as the input name in the command name.
Modifications:
Result:
Two build commands generated by plugins can have the same name and the build will still work.
rdar://79872414