`mach package` treated as a steps.Compile #503

lucasloisp · 2016-10-05T00:16:46Z

@aneeshusa: as per issue #372, both make_step now have a heuristic for checking that the argument passed to mach (mach_arg) is 'package' and assigns step_class = steps.Compile

This change is

highfive · 2016-10-05T00:16:48Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @aneeshusa (or someone else) soon.

aneeshusa

Looks good overall. A few nits on the commit message:

Keep the title (first line) to around 50 chars or less
Add some motivation to the body. Why are we making this change?

aneeshusa · 2016-10-05T01:20:04Z

buildbot/master/files/config/factories.py

@@ -106,6 +106,8 @@ def make_step(self, command):
                step_desc = [mach_arg]
                if re.match('build(-.*)?', mach_arg):
                    step_class = steps.Compile
+                if re.match('package', mach_arg):


nit: use elif instead of if

I would have sworn I used elif. I'll change it

aneeshusa · 2016-10-05T01:20:04Z

buildbot/master/files/config/factories.py

@@ -251,6 +253,8 @@ def make_step(self, command):
                step_desc = [mach_arg]
                if re.match('build(-.*)?', mach_arg):
                    step_class = steps.Compile
+                if re.match('package', mach_arg):


nit: use elif instead of if

aneeshusa · 2016-10-05T02:19:24Z

Commit message looks much better :) A few more nits:

We actually have mach package for more platforms now, not just Android, so amend the beginning to something like Even when this command fails, Buildbot still goes on to run the next step, ....
The parenthetical with the issue reference is unneeded as GitHub already will make it easy to get to the issue from the commit, so remove it.
grammar nit: check for it on the -> check for it in the

Also it looks like a merge commit sneaked in there, so go ahead and squash everything into one commit.


        Treat './mach package' as a steps.Compile

Even when this command fails, Buildbot still goes on to run the next step, because it is run as a simple ShellCommand. Instead, we now check for it in the `make_step` function and use `steps.Compile` to create the step as it is not only a closer semantic match (we're "compiling" the package) but it also makes it so when it fails Builbot doesn't ignore it or move on to the next step.

lucasloisp · 2016-10-12T20:55:29Z

@aneeshusa Anything I should change or is the change ready to be approved?

aneeshusa · 2016-10-12T21:08:04Z

@lucasloisp Sorry, I didn't get a notification that you had pushed new changes for some reason! This looks great, thanks for the PR :)

@bors-servo r+

bors-servo · 2016-10-12T21:08:06Z

📌 Commit 9296512 has been approved by aneeshusa

bors-servo · 2016-10-12T21:08:08Z

⌛ Testing commit 9296512 with merge 13c0fce...


        Auto merge of #503 - lucasloisp:mach_package_as_compile_step, r=anees…

@aneeshusa

…husa `mach package` treated as a steps.Compile @aneeshusa: as per issue #372, both `make_step` now have a heuristic for checking that the argument passed to mach (`mach_arg`) is `'package'` and assigns `step_class = steps.Compile`  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/503)

bors-servo · 2016-10-12T22:15:09Z

☀️ Test successful - status-travis

highfive assigned aneeshusa Oct 5, 2016

highfive added the S-awaiting-review label Oct 5, 2016

aneeshusa suggested changes Oct 5, 2016

View changes

aneeshusa added S-needs-code-changes and removed S-awaiting-review labels Oct 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 5, 2016

aneeshusa added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Oct 5, 2016

lucasloisp force-pushed the lucasloisp:mach_package_as_compile_step branch from 5a22763 to 0412ec7 Oct 5, 2016

highfive added S-awaiting-review and removed S-needs-code-changes labels Oct 5, 2016

lucasloisp force-pushed the lucasloisp:mach_package_as_compile_step branch from 0412ec7 to 9296512 Oct 5, 2016

jdm removed the S-needs-squash label Oct 12, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Oct 12, 2016

bors-servo merged commit 9296512 into servo:master Oct 12, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Oct 12, 2016

bors-servo mentioned this pull request Oct 12, 2016

Remove old DynamicServoFactory which reads from saltfs #504

Merged

jdm mentioned this pull request Oct 23, 2016

Use steps.Compile for `mach package` on Buildbot #372

Closed

lucasloisp deleted the lucasloisp:mach_package_as_compile_step branch Oct 24, 2016

servo / saltfs

`mach package` treated as a steps.Compile #503

`mach package` treated as a steps.Compile #503

lucasloisp commented Oct 5, 2016 •

edited by larsbergstrom

highfive commented Oct 5, 2016

aneeshusa left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Oct 5, 2016

lucasloisp commented Oct 12, 2016

aneeshusa commented Oct 12, 2016

bors-servo commented Oct 12, 2016

bors-servo commented Oct 12, 2016

bors-servo commented Oct 12, 2016

servo / saltfs

Join GitHub today

`mach package` treated as a steps.Compile #503

`mach package` treated as a steps.Compile #503

Conversation

lucasloisp commented Oct 5, 2016 • edited by larsbergstrom

highfive commented Oct 5, 2016

aneeshusa left a comment

This comment has been minimized.

aneeshusa Oct 5, 2016

This comment has been minimized.

lucasloisp Oct 5, 2016

This comment has been minimized.

aneeshusa Oct 5, 2016

aneeshusa commented Oct 5, 2016

lucasloisp commented Oct 12, 2016

aneeshusa commented Oct 12, 2016

bors-servo commented Oct 12, 2016

bors-servo commented Oct 12, 2016

bors-servo commented Oct 12, 2016

lucasloisp commented Oct 5, 2016 •

edited by larsbergstrom