Windows changes to make the HOME env var work #442

larsbergstrom · 2016-07-19T20:21:00Z

There are two big changes here:

Make setting the HOME env var work properly, filling in the builder name
Just allow exceptions to fail to load buildbot and log normally. Right now, the print is being eaten by something, so it's super hard to debug failures with dynamic steps.

This change is

aneeshusa · 2016-07-20T02:26:20Z

I mentioned this in #316 (comment) but unswallowing the exception here won't help. The exception Buildbot encounters is actually internal and occurs after our dynamic step is finished, when Buildbot tries to load the next step. We aren't respecting some of Buildbot 8's internal invariants, causing the exception. You can find out more about this by looking at the log files/digging into the source code of buildbot and the buildbot-travis project I linked.

aneeshusa · 2016-07-20T02:26:37Z

buildbot/master/files/config/factories.py

+            step_env += envs.Environment({
+                # Set home directory, to avoid adding `cd` command every time
+                'HOME': r'C:\buildbot\slave\{}\build'.format(
+                self.builder_name),


Add 4 spaces of indentation.

Also, the ), can then go on the next line for balance.

aneeshusa · 2016-07-20T02:26:45Z

buildbot/master/files/config/factories.py

+            step_env += envs.Environment({
+                # Set home directory, to avoid adding `cd` command every time
+                'HOME': r'C:\buildbot\slave\{}\build'.format(
+                self.builder_name),


Add 4 spaces of indentation.

Also, the ), can then go on the next line for balance.

aneeshusa · 2016-07-20T02:27:13Z

buildbot/master/files/config/factories.py

+                # Set home directory, to avoid adding `cd` command every time
+                'HOME': r'C:\buildbot\slave\{}\build'.format(
+                self.builder_name),
+                })


Subtract 4 spaces of indentation.

aneeshusa · 2016-07-20T02:27:24Z

buildbot/master/files/config/factories.py

+                # Set home directory, to avoid adding `cd` command every time
+                'HOME': r'C:\buildbot\slave\{}\build'.format(
+                self.builder_name),
+                })


Subtract 4 spaces of indentation.

larsbergstrom · 2016-07-20T03:13:53Z

Latest commit fixes servo/servo#12231

aneeshusa · 2016-07-20T03:17:21Z

buildbot/master/files/config/factories.py

+                    # s3cmd on Windows only works within msys
+                    step_env += envs.Environment({
+                        'MSYSTEM': 'MSYS',
+                        })


-4 spaces indentation please

Also please add this to the other make_step function.


        Enable proper upload of Windows nightlies via s3cmd

larsbergstrom · 2016-07-20T20:34:51Z

OK, I think this addresses all of the formatting requests.

Note that I left the removal of the print statement and try block around the dynamic steps because otherwise right now there's no way (AFAIK) to debug the exception, as it gets swallowed, printed to /dev/null, and the only way you can tell is when you queue a job to that builder the steps look "off." @edunham mentioned she might switch it to use buildbot's logging infrastructure instead.

aneeshusa · 2016-07-20T21:10:26Z

Please keep the print and try block intact. The log you need to look at is /home/servo/buildbot/master/twistd.log on servo-master1; search for setText to jump to the exception trace. Removing the try will not help because it exception is not thrown within our dynamic step, but after that step is finished, in the internals of buildbot when it tries to start the next step.

+1 for integrating with buildbot's logging infrastructure in the future though.

aneeshusa · 2016-07-20T21:10:50Z

BTW, I started looking into that exception if you want to chat on IRC about it.

aneeshusa · 2016-07-20T21:11:52Z

buildbot/master/files/config/factories.py

@@ -109,6 +113,17 @@ def make_step(self, command):
            elif arg == './etc/ci/upload_nightly.sh':
                step_kwargs['logEnviron'] = False
                step_env += envs.upload_nightly
+                if self.is_windows:
+                    # s3cmd on Windows only works within msys
+                    step_env['MSYSTEM'] = 'MSYS'


Use another envs.Environment here.

hrm, when I was using envs.Environment, the debugging seemed to indicate that it wasn't overwriting it - we were still getting the old value in testing. I can give it another try, though, and see what happens.

This is a stylistic change we can always make later, so I'm OK with this as-is if we need the PR for windows nightlies now.

Yeah, it's already in deployment as-formatted, so I'm kinda tempted to get this landed (since I already feel bad about having an ad-hoc branch in production).

aneeshusa · 2016-07-20T21:31:37Z

After some IRC chat it turns out I was looking at the wrong thing for the exception handling and removing the try is the best thing to do here; this is r=me with or without using an envs.Environment for the MSYSTEM and PATH variables.

larsbergstrom · 2016-07-20T21:32:00Z

@bors-servo r=aneeshusa

bors-servo · 2016-07-20T21:32:01Z

📌 Commit 239bcf8 has been approved by aneeshusa

bors-servo · 2016-07-20T21:32:03Z

⌛ Testing commit 239bcf8 with merge 8f6afa9...


        Auto merge of #442 - larsbergstrom:more_windows, r=aneeshusa

@edunham

Windows changes to make the HOME env var work There are two big changes here: 1) Make setting the `HOME` env var work properly, filling in the builder name 2) Just allow exceptions to fail to load buildbot and log normally. Right now, the `print` is being eaten by something, so it's super hard to debug failures with dynamic steps. r? @edunham  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/442)

bors-servo · 2016-07-20T21:50:33Z

☀️ Test successful - travis

aneeshusa reviewed Jul 20, 2016
View changes

aneeshusa added the S-needs-code-changes label Jul 20, 2016

aneeshusa reviewed Jul 20, 2016
View changes

Enable proper upload of Windows nightlies via s3cmd

Loading status checks…

239bcf8

larsbergstrom force-pushed the larsbergstrom:more_windows branch from 2295426 to 239bcf8 Jul 20, 2016

aneeshusa reviewed Jul 20, 2016
View changes

bors-servo merged commit 239bcf8 into servo:master Jul 20, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

bors-servo mentioned this pull request Jul 20, 2016

Add description to buildbot steps #438

Merged

aneeshusa mentioned this pull request Jul 22, 2016

Builds not being uploaded properly by s3cmd servo/servo#12231

Closed

servo / saltfs

Windows changes to make the HOME env var work #442

Windows changes to make the HOME env var work #442

larsbergstrom commented Jul 19, 2016 •

edited

aneeshusa commented Jul 20, 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.

larsbergstrom commented Jul 20, 2016

This comment has been minimized.

This comment has been minimized.

larsbergstrom commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jul 20, 2016

larsbergstrom commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

servo / saltfs

Join GitHub today

Windows changes to make the HOME env var work #442

Windows changes to make the HOME env var work #442

Conversation

larsbergstrom commented Jul 19, 2016 • edited

aneeshusa commented Jul 20, 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.

larsbergstrom commented Jul 20, 2016

This comment has been minimized.

This comment has been minimized.

larsbergstrom commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

aneeshusa commented Jul 20, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

aneeshusa commented Jul 20, 2016

larsbergstrom commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

bors-servo commented Jul 20, 2016

larsbergstrom commented Jul 19, 2016 •

edited