I think this code is fine, but the implementation seems like it should be more general. For example:

• A build step which stops on buildable type?
• Optional preconditions for all build steps which let you select buildable type, etc.?
• A build step which runs a different build plan, so the commit and non-commit plans can "call" the common code?
• Variable-replacement in Phragment publishing, so you create /submitted/ vs /proposed/ fragments? (This is already possible, it looks like.)

Specifically, the concrete issues I see are:

1. This doesn't generalize at all; we'll have to do the same thing if we run into this problem again.
2. We'll still upload the file even if we don't create a fragment, and there's no way to prevent this. This is wasteful if we don't care about the file.
3. If this step emits artifacts (presumably, it should emit a "fragment" artifact eventually) the artifacts may not actually be emitted if the step is conditional. That is, we may have phantom artifacts which sometimes exist in later build steps and sometimes do not.

Of these, (3) seems like the biggest problem: do we need to deal with the possibility that build steps may succeed but not emit their artifacts? It would be much simpler if we could design the system so this was impossible -- that is, a build step can always look at previous build steps and know which artifacts will be available.

Of the solutions above, a "run build plan" build step would do this, as would stopping the whole build or changing where the fragment gets uploaded. Preconditions would not be sufficient to avoid phantom artifacts.

We could also accept phantom artifacts, but their behavior should be well-defined (I think the only reasonable behavior is to skip all subsequent build steps which require the artifact?). This may be difficult because it's possible to create implicit dependencies between build steps (e.g., step A leaves some binary in the working copy, and step B depends on it).

The easiest workaround today would be to upload to a variable fragment, e.g.:

/Project/${buildable.type}/blah/blah.exe

Then you'll end up with two fragments, one for commits (which you care about) and one for revisions (which you can just ignore). Once we figure out the answers to all the other questions we could develop a better fix.

I lean slightly toward an "Apply Build Plan" build step, since I think that will be useful in general. However, I worry that it won't be sufficient and that we'll have to tackle phantom artifacts anyway.

If the workaround above is good for now, let's sit on this for a bit? I think some of the infrastructure changes which are slated for development (like parallelizing the workers) might create more clarity here without locking us into anything.

This kind of optional execution isn't really practical for steps that produce artifacts, especially if there are build steps down the line that are relying on them. "Publish Fragment" doesn't actually create an artifact from it's execution, so we're kind of safe in this context.

Generalizing this functionality does however introduced the mentioned problems; we can't generalize this across all build steps without also considering what happens when you are missing an artifact. Personally I'd be inclined to just fail the build if you try and load an artifact that doesn't exist (because the build step that produced it didn't run).

I think this is probably the most expected reaction too; if I try and upload a file artifact from a remote machine and it doesn't exist, I expect failure in the same way that if Phragment can't access the file artifact I expect failure.

According to the code, it looks like loadDrydockLease and loadPhabricatorFile on HarbormasterBuildArtifact will already throw an exception if the artifact is not found, so this should already result in the expected behaviour.