Add development-time assertions to catch errors where two llbuild commands are created with the same name

[abertelrud]

Otherwise this results in silently replacing some commands in the graph. We can discuss whether a future improvement would be to make these internal errors reported at runtime.

Motivation:

Without these assertions, adding two or more commands with the same name result in only the last of them remaining, which can lead to hard-to-diagnose build failures. I have to not come across any cases of this happening intentionally, but there is currently a Linux-specific test failure that I need to look closer at. It could be a bug that this is catching or could be that this needs to be made more lenient. Even if the replacement behavior is intentional, it seems like the kind of thing that should be done explicitly in the code. This was motivated by an unintentional replacement of a build command.

Modifications:

• add development-time assertions to catch replacing commands in the llbuild manifest

Result:

In debug builds and in tests, any replacement of llbuild commands will be made apparent.

[abertelrud]

@swift-ci please smoke test

[abertelrud]

So it seems that this can happen also on macOS, at least when running unit tests manually using swift test. The problem happens in BuildTests.BuildPlanTests.testExplicitSwiftPackageBuild, where the addSwiftFrontendCmd() command is called twice for the same module _Concurrency. It's invoked from addSwiftDriverJobs(). There are some minor differences in the command line but it largely seems to be the same command. In particular, the output path is the same in both cases.

The inputs are not the same, however; in one case LLBuildManifest.Node(name: "<_Concurrency-dependencies-1.json>" and in the other LLBuildManifest.Node(name: "<_Concurrency-dependencies-2.json>". These are both virtual so it might be that they end up having the same transitive dependencies, but I would need to look closer at that. They are being generated for different module names that both include _Concurrency.

@artemcm do you know if this is expected? I can elaborate more about this, but wanted to first check it it was expected that multiple LLBuild manifest commands would be generated with the same name and only one of them kept. It's possible that this is harmless, but it could also be an intermittent bug if there is some nondeterminism to the order in which these commands are created.

[artemcm]

Yes, with explicit modules (the test in question), it is expected that we may produce multiple jobs to build the same module. If we're separately planning any two targets, A, and B, they may have common module dependencies. Implicit imports (Swift and _Concurrency) are one such example that will be common to most Swift modules.

The desired outcome is that these identical commands get de-duplicated by making sure they result in only one executable job in LLBuild. The fact that one of the input files here differs (in name, not in content...) may prevent that though, and is something we should fix.

Even if the replacement behavior is intentional, it seems like the kind of thing that should be done explicitly in the code.

This is a great point, we should make the code do this explicitly.

[abertelrud]

Even if the replacement behavior is intentional, it seems like the kind of thing that should be done explicitly in the code.

This is a great point, we should make the code do this explicitly.

Sounds good. I'll leave this PR open in Draft mode until then, so that we can get these development-time checks in after that lands, in order to catch future problems.

[abertelrud]

@artemcm Is this something we can bring back at this point? i.e. has the issue with SwiftDriver creating duplicate commands been resolved?

[artemcm]

@artemcm Is this something we can bring back at this point? i.e. has the issue with SwiftDriver creating duplicate commands been resolved?

The missing piece is not preventing SwiftDriver from creating duplicate commands, but rather filtering those duplicate commands before they make it down to LLBuild.

SwiftDriver should not have the context to know whether a job it is emitting is duplicate as part of the larger build because it only handles building a single module at a time. Maybe when translating the driver's Job to a build manifest command would be a good place for such deduplication?

[abertelrud]

@artemcm Is this something we can bring back at this point? i.e. has the issue with SwiftDriver creating duplicate commands been resolved?

The missing piece is not preventing SwiftDriver from creating duplicate commands, but rather filtering those duplicate commands before they make it down to LLBuild.

SwiftDriver should not have the context to know whether a job it is emitting is duplicate as part of the larger build because it only handles building a single module at a time. Maybe when translating the driver's Job to a build manifest command would be a good place for such deduplication?

It should happen somewhere above the manifest generation, because otherwise there isn't a good place to detect accidental overwriting of previous commands (as happened in the original bug that prompted this PR). Basically, whoever is emitting llbuild commands should use some criteria to determine whether two commands it has generated are the same, and if so, not emit the second (or third, etc) ones. If SwiftDriver doesn't know, then whatever translates Jobs to llbuild commands could do it.

[abertelrud]

@artemcm How do we move forward on this? Having the default behavior for llbuild manifests be that commands get silently dropped seems bad (this whole thing started because of a bug in plugins that was due to accidentally overwriting multiple commands). I could see an option to allow it, or deduping at a higher level, or a variety of other solutions that don't involve silently dropping commands on the ground in the default case. That's just more bugs awaiting to happen.

[artemcm]

@abertelrud, I think deduplication at a higher level is the way to address this.
Seeing as this is only relevant for explicit module builds (in implicit module builds we do not have identical commands from different targets so we want to catch those as errors), let me see if I can add a simple filtering mechanism there.

[abertelrud]

@abertelrud, I think deduplication at a higher level is the way to address this.
Seeing as this is only relevant for explicit module builds (in implicit module builds we do not have identical commands from different targets so we want to catch those as errors), let me see if I can add a simple filtering mechanism there.

Thanks @artemcm that sounds great. If we do need an option (which clients would have to opt into) to say "yes replace any existing command with the same name" then that would work too, but if it makes sense to do at a higher level I think that would be preferable (as "Builder" patterns don't usually involve replacing parts of what was already added to the builder during the same build).

[artemcm]

@abertelrud #3769 is an attempt to ensure we don't add duplicates to the LLBuild manifest.

[abertelrud]

#3587
@swift-ci please smoke test