Page MenuHomePhabricator

Implement artifact dependencies and parallisation in Harbormaster
AbandonedPublic

Authored by hach-que on Jul 3 2014, 5:13 AM.
Tags
None
Referenced Files
F14078887: D9806.diff
Fri, Nov 22, 5:45 AM
Unknown Object (File)
Wed, Nov 20, 6:53 AM
Unknown Object (File)
Tue, Nov 19, 3:47 AM
Unknown Object (File)
Mon, Nov 18, 3:47 PM
Unknown Object (File)
Mon, Nov 18, 2:36 PM
Unknown Object (File)
Mon, Nov 18, 11:16 AM
Unknown Object (File)
Sat, Nov 16, 9:48 AM
Unknown Object (File)
Sun, Nov 10, 5:49 AM
Subscribers

Details

Summary

This adds artifact dependencies and parallisation to Harbormaster. Each build step now has an output artifact called "Build State", where the key is the PHID of the build step. Build steps have a depends_on field which indicates what other build states they depend on.

In the plan editor, it looks like this:

harbormastervisualize4.png (620×799 px, 41 KB)

To keep things simple and to ensure you can't create circular dependencies without knowing about it, the user still needs to order their build steps such that a build step only refers to artifacts before it. Otherwise you get the 'Artifact not available at this time during the build' message that you currently get.

harbormastervisualize5.png (273×798 px, 14 KB)

The Depends On field does not display for the first step in the build.

NOTE: This does not yet consider non-build state dependencies. i.e. if you have a "Publish Fragment" step that relies on a file artifact, it won't be included in the dependency calculations.
Test Plan

Configured various Sleep steps with different dependencies.

Diff Detail

Repository
rP Phabricator
Branch
harbormaster-dependencies
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1513
Build 1513: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que retitled this revision from to Implement artifact dependencies and parallisation in Harbormaster.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)
hach-que added a reviewer: epriestley.
src/applications/harbormaster/step/HarbormasterBuildStepImplementation.php
136

This should omit the type to allow null to be passed in; it's already fixed in D9809 if you plan on accepting that, otherwise I can backport the fix to this diff.

hach-que edited the test plan for this revision. (Show Details)
hach-que edited edge metadata.
hach-que updated this object.
hach-que edited the test plan for this revision. (Show Details)

Use step name for showing dependencies.

This now uses the step name for showing dependencies, and moves them into their own "Depends On" section (seperate from "Input Artifacts"). Because steps are now named, it also hides the output build state since users don't need to know the PHID any more.

Backport fix from D9809 since that now has changes planned.

I think this is mostly reasonable.

  • Datasource stuff should wait for T4420 and then use the new modular stuff.
  • Retaining the ordering stuff doesn't make sense, but it looks like D9847 cleans that up.
  • I'm not sure dependencies should be artifacts. Possibly, they should be a separate thing and artifacts should create dependencies rather than dependencies creating artifacts? Let me look at D9847...
  • I don't understand the "$current_step might be null" thing (see inline).
  • We need to migrate existing plans, but maybe D9847 covers that.
  • We should have real cycle detection (maybe after D9847), since the database can definitely get into a cycle even if the UI tries to prevent it. Currently, I can make "B depends on A", reorder them, then set "A depends on B", I think.

Let me read through D9847 and see how much of this that resolves.

src/applications/harbormaster/controller/HarbormasterStepEditController.php
160–166

After D9847, presumably this should just be everything?

src/applications/typeahead/controller/PhabricatorTypeaheadStepDependenciesDatasourceController.php
28–33

Why is it valid to have no $current_step here?