Convert Windows builders to use DynamicServoFactory #426

UK992 · 2016-07-05T11:56:03Z

I'm not sure if this is the right approaches, but i try.

For process kill i used powershell kill, which works from bash and cmd, but needs admin privileges like any other process kill command.

This change is

aneeshusa · 2016-07-05T11:59:46Z

Thanks for tackling this!

aneeshusa · 2016-07-05T12:00:24Z

buildbot/master/files/config/environments.py

@@ -31,6 +31,7 @@ def __add__(self, other):

 build_windows = build_common + Environment({
    'CARGO_HOME': '/home/Administrator/.cargo',
+    'HOME': r'C:\buildbot\slave\windows\build',


This seems like an odd choice of $HOME. Why do we need to set it, and why this value?

this set home directory before bash login, so it not needed to add cd command before every command.

That's a reasonable hack, but please leave a comment with that explanation for future reference.

aneeshusa · 2016-07-05T12:01:12Z

buildbot/master/files/config/steps.yml

@@ -76,3 +76,12 @@ arm64:
  - ./mach build --rel --target=aarch64-unknown-linux-gnu
  - bash ./etc/ci/lockfile_changed.sh
  - bash ./etc/ci/manifest_changed.sh
+
+windows:
+  - ./mach build -d -v


Let's do ./mach build --dev for consistency. Also, please rename to windows-dev as well.

aneeshusa · 2016-07-05T12:02:54Z

buildbot/master/files/config/factories.py

-        step_kwargs['command'] = command
+
+        # Add bash -l before every command on Windows builders
+        bash_args = ["bash", "-l"] if is_windows() else []


We had ["bash", "-l", "-c"] earlier - missed the "-c".

i leave -c, to avoid quoting every command, i've tested with appveyor and it work that way too.

aneeshusa · 2016-07-05T12:05:40Z

buildbot/master/files/config/factories.py

@@ -216,6 +235,10 @@ def __init__(self, builder_name, environment):
        ])


+def is_windows():
+    return platform.system() == "Windows"


This doesn't work because this code runs on the Buildbot master (Linux). We need to somehow interrogate the builder properties and use that throughout the build. I don't know exactly how to do that.

FYI, this is the hard part.

with new approaches not detect windows builder based on builder name

aneeshusa · 2016-07-05T12:05:49Z

buildbot/master/files/config/factories.py

+        if is_windows():
+            pkill_command = ["powershell", "kill", "-n", "servo"]
+        else:
+            pkill_command = ["pkill", "-x", "servo"]


Would be nice to factor this out into a little helper function:

def make_pkill_command(target): ... make_pkill_command("servo")

Another option is a script in etc/ci, which neatly steps around the "how to detect Windows" problem.

aneeshusa · 2016-07-05T12:09:04Z

buildbot/master/files/config/factories.py

+                if is_windows():
+                    step_env = copy.deepcopy(envs.upload_nightly_windows)
+                else:
+                    step_env = copy.deepcopy(envs.upload_nightly)


We should just do step_env += envs.upload_nightly and get rid of envs.upload_nightly_windows - would have mentioned this on the earlier PR if it wasn't right before release :)

aneeshusa · 2016-07-05T12:51:27Z

buildbot/master/files/config/factories.py

@@ -1,6 +1,7 @@
 import copy
 import os.path
 import re
+import platform


No longer needed.

aneeshusa · 2016-07-05T12:52:16Z

buildbot/master/files/config/factories.py

@@ -58,31 +59,37 @@ class DynamicServoFactory(ServoFactory):

    def __init__(self, builder_name, environment):
        self.environment = environment
+        is_windows = True if re.match('windows(.*)', builder_name) else False


I think this would make sense as a field on self.

aneeshusa · 2016-07-05T12:53:12Z

buildbot/master/files/config/factories.py

@@ -215,6 +228,12 @@ def __init__(self, builder_name, environment):
                                 "etc/ci/buildbot_steps.yml")
        ])

+def make_pkill_command(target, is_windows):


Similarly, I think this makes sense as a class method - please mind the duplication for now :)

i don't quite understand, should i add this function to classes DynamicServoYAMLFactory and DynamicServoFactory?

EDIT: i added function to both classes, but i think this is not what you mean. I can restore back to previous state, if needed.

aneeshusa · 2016-07-05T12:54:45Z

I will finish reviewing this either later today or tomorrow.

aneeshusa · 2016-07-06T01:47:39Z

buildbot/master/files/config/factories.py

@@ -58,31 +58,38 @@ class DynamicServoFactory(ServoFactory):

    def __init__(self, builder_name, environment):
        self.environment = environment
+        self.is_windows = True if re.match('windows(.*)',


We can do self.is_windows = re.match(...) is not None directly instead. Also, no need for the parentheses around the .*.

aneeshusa · 2016-07-06T01:49:56Z

buildbot/master/files/config/factories.py


        # util.BuildFactory is an old-style class so we cannot use super()
        # but must hardcode the superclass here
        ServoFactory.__init__(self, pkill_step + dynamic_steps)

-    def make_step(self, command):
+    def make_step(self, command, is_windows):


We can just read from self.is_windows instead of using an extra argument.

aneeshusa · 2016-07-06T01:50:10Z

buildbot/master/files/config/factories.py


        step_kwargs['env'] = step_env
        return step_class(**step_kwargs)

+    def make_pkill_command(self, target, is_windows):


Use self.is_windows here too.

aneeshusa · 2016-07-06T02:34:43Z

buildbot/master/files/config/factories.py

        try:
            config_dir = os.path.dirname(os.path.realpath(__file__))
            yaml_path = os.path.join(config_dir, 'steps.yml')
            with open(yaml_path) as steps_file:
                builder_steps = yaml.safe_load(steps_file)
            commands = builder_steps[builder_name]
-            dynamic_steps = [self.make_step(command) for command in commands]
+            dynamic_steps = [self.make_step(command, self.is_windows)


No need to pass self.is_windows to make_step().

aneeshusa · 2016-07-06T02:36:04Z

buildbot/master/files/config/factories.py

-                                         decodeRC={0: SUCCESS, 1: SUCCESS})]
+        pkill_step = [steps.ShellCommand(
+            command=self.make_pkill_command("servo"),
+            decodeRC={0: SUCCESS, 1: SUCCESS}


Does this decodeRC apply for the powershell command as well? I think it would be better if the make_pkill_command function returned a steps.ShellCommand directly instead of just the command array.

yes it does, if powershell kill doesnt find process, exit with code 1

aneeshusa · 2016-07-06T03:19:24Z

buildbot/master/files/config/factories.py

        try:
            config_dir = os.path.dirname(os.path.realpath(__file__))
            yaml_path = os.path.join(config_dir, 'steps.yml')
            with open(yaml_path) as steps_file:
                builder_steps = yaml.safe_load(steps_file)
            commands = builder_steps[builder_name]
-            dynamic_steps = [self.make_step(command) for command in commands]
+            dynamic_steps = [self.make_step(command)
+                             for command in commands]


Put this back on one line as it was before.

aneeshusa · 2016-07-06T03:19:41Z

buildbot/master/files/config/factories.py


        step_kwargs['env'] = step_env
        return step_class(**step_kwargs)

+    def make_pkill_command(self, target):


Let's rename to make_pkill_step since it returns a Step.

aneeshusa · 2016-07-06T03:40:35Z

LGTM to me after squash. Nice thinking with using a regex on the builder name to determine is_windows - I thought we were going to have to use complicated Buildbot properties or something.


        Convert Windows builders to use DynamicServoFactory


        Rename builder to windows-dev


        Remove env.upload_nightly_windows

UK992 · 2016-07-06T03:45:59Z

Squashed!

aneeshusa · 2016-07-06T03:54:55Z

@bors-servo r+
Thanks for handling not one but two TODOs!

bors-servo · 2016-07-06T03:54:56Z

📌 Commit af95e4e has been approved by aneeshusa

bors-servo · 2016-07-06T03:54:58Z

⌛ Testing commit af95e4e with merge d13f42f...


        Auto merge of #426 - UK992:win-builders, r=aneeshusa

Convert Windows builders to use DynamicServoFactory I'm not sure if this is the right approaches, but i try. For process kill i used ```powershell kill```, which works from bash and cmd, but needs admin privileges like any other process kill command.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/426)

bors-servo · 2016-07-06T04:09:20Z

☀️ Test successful - travis

aneeshusa reviewed Jul 5, 2016
View changes

UK992 force-pushed the UK992:win-builders branch 4 times, most recently from c553573 to c2080f9 Jul 5, 2016

aneeshusa reviewed Jul 6, 2016
View changes

UK992 added 3 commits Jul 3, 2016

Convert Windows builders to use DynamicServoFactory

a57df4c

Rename builder to windows-dev

ac58729

Remove env.upload_nightly_windows

Loading status checks…

af95e4e

UK992 force-pushed the UK992:win-builders branch from 038c382 to af95e4e Jul 6, 2016

bors-servo merged commit af95e4e into servo:master Jul 6, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

aneeshusa mentioned this pull request Jul 8, 2016

Run pkill on the Windows builders #351

Closed

servo / saltfs

Join GitHub today

Convert Windows builders to use DynamicServoFactory #426

Convert Windows builders to use DynamicServoFactory #426

Conversation

UK992 commented Jul 5, 2016 • edited by larsbergstrom

aneeshusa commented Jul 5, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

UK992 Jul 5, 2016 • edited

aneeshusa commented Jul 5, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jul 6, 2016

UK992 commented Jul 6, 2016

aneeshusa commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

bors-servo commented Jul 6, 2016

UK992 commented Jul 5, 2016 •

edited by larsbergstrom

UK992 Jul 5, 2016 •

edited