Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix ./mach run on Windows #9415
Conversation
highfive
commented
Jan 25, 2016
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @larsbergstrom (or someone else) soon. |
|
@adamncasey We can close #9408 and I'll just review and test this one as a whole, if that works for you. Thanks for all your work on Windows - I really appreciate it! |
|
Hrm, when I run with Also, on the first commit, I think we would need to install virtualenv for the mingw64 python in order for this to work. |
|
Aha, I've managed to get this working locally - user error on my part. I'll also land the other python-related fix because it works with the current instructions. I suspect what's needed to make the "mingw64 python" work is installing virtualenv/easy_install for mingw64. @bors-servo r+ |
|
|
Fix ./mach run on Windows This branch also includes the commit being reviewed in #9408 . Is it OK to just wait for that one to merge, or should I separate them? This patch adds the BIN_SUFFIX to the servo executable name when doing ./mach run. However just doing this caused subprocess to error due to some unicode symbols in PATH. To fix this, I pulled a peice of code from mozilla-central which fixes this problem there. Source: https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/mixin/process.py#108 Revisiting this now (originally developed this a few weeks ago), perhaps we should be using https://github.com/servo/servo/blob/master/python/mach/mach/mixin/process.py instead of subprocess to give us all of this crossplatform process execution support. I think that should be a nice to have though - this patch at least gets a necessary development command working on windows. Tested on Windows and Ubuntu. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9415) <!-- Reviewable:end -->
|
|
|
Looks like a reftest failed. Is this a known intermittent failure? Tests with unexpected results: |
|
@bors-servo retry |
|
|
|
|
Prepend PATH extras instead of append (Fix Issue #9437) Should fix multirust issue #9437 which appeared after #9415 landed. @jdm to confirm <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9440) <!-- Reviewable:end -->
… #9437) (from adamncasey:fix-multirust); r=jdm Should fix multirust issue servo/servo#9437 which appeared after servo/servo#9415 landed. jdm to confirm Source-Repo: https://github.com/servo/servo Source-Revision: 03b763c2c5b873ccc454278a55737bc37e940e97 UltraBlame original commit: a734c8ee197848ee30c80d7dd9dbf51e84e2eb4a
… #9437) (from adamncasey:fix-multirust); r=jdm Should fix multirust issue servo/servo#9437 which appeared after servo/servo#9415 landed. jdm to confirm Source-Repo: https://github.com/servo/servo Source-Revision: 03b763c2c5b873ccc454278a55737bc37e940e97 UltraBlame original commit: a734c8ee197848ee30c80d7dd9dbf51e84e2eb4a
… #9437) (from adamncasey:fix-multirust); r=jdm Should fix multirust issue servo/servo#9437 which appeared after servo/servo#9415 landed. jdm to confirm Source-Repo: https://github.com/servo/servo Source-Revision: 03b763c2c5b873ccc454278a55737bc37e940e97 UltraBlame original commit: a734c8ee197848ee30c80d7dd9dbf51e84e2eb4a
adamncasey commentedJan 25, 2016
This branch also includes the commit being reviewed in #9408 . Is it OK to just wait for that one to merge, or should I separate them?
This patch adds the BIN_SUFFIX to the servo executable name when doing ./mach run.
However just doing this caused subprocess to error due to some unicode symbols in PATH.
To fix this, I pulled a peice of code from mozilla-central which fixes this problem there. Source: https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/mixin/process.py#108
Revisiting this now (originally developed this a few weeks ago), perhaps we should be using https://github.com/servo/servo/blob/master/python/mach/mach/mixin/process.py instead of subprocess to give us all of this crossplatform process execution support. I think that should be a nice to have though - this patch at least gets a necessary development command working on windows.
Tested on Windows and Ubuntu.