Win32 support #9385

larsbergstrom · 2016-01-20T14:57:09Z

r? @frewsxcv for python stuff
r? @pcwalton for the "remove usage of Gaol" stuff for Win32
r? anybody else for misc cargo.lock updates, etc.

This replaces #7878.

This works best with servo/mozjs#71, too, to enable static linking, but can be run without (via some PATH hackery).

The instructions are here, and will be added to a .md file in the repo once the mozjs changes also land:
https://hackpad.com/Servo-on-Windows-C1LPcI2bP25

I'd like to get these changes landed because I've been rebasing them for months, they're otherwise quite stable, and don't affect our other platforms and targets.


        win32: use fontconfig/freetype on windows as well (for now)


        win32: make sleep_microseconds for backoff just do a win32 Sleep(0)


        win32: look for python.exe and variants on win32 in style/build.rs


        win32: mach and build command fixes

- Add SERVO_USE_NIGHTLY_RUST env var to use the latest rust/cargo nightly snapshot - Fix up looking for cargo binary (in cargo/bin/cargo, not bin/cargo) - Fix up win32 executable checking (use .exe suffix) - fix up win32 PATH handling (subprocess must use shell=True for PATH change to be honored)


        win32: force PATH to be non-unicode on win32, if virtualenv munges it


        Cargo updates and small workqueue change

highfive · 2016-01-20T14:57:13Z

Heads up! This PR modifies the following files:

@bholley: components/style/build.rs

highfive · 2016-01-20T14:57:14Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify gfx and style code, but no tests are modified. Please consider adding a test!

jdm · 2016-01-20T15:48:09Z

I left the python stuff and the sandboxing stuff alone.

Reviewed 5 of 14 files at r1, 2 of 3 files at r2, 4 of 9 files at r4.
Review status: 11 of 24 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

components/gfx/Cargo.toml, line 88 [r4] (raw file):
This should use crates.io instead.

components/servo/Cargo.lock, line 72 [r4] (raw file):
Uh...

components/servo/Cargo.lock, line 913 [r4] (raw file):
Two kernel32-sys packages.

components/servo/Cargo.lock, line 1660 [r4] (raw file):
Two servo-fontconfigs? I'm surprised that test-tidy doesn't complain here.

components/util/Cargo.toml, line 71 [r4] (raw file):
Let's use the more recent version instead.

ports/gonk/Cargo.lock, line 54 [r4] (raw file):
Revert.

Comments from the review on Reviewable.io

larsbergstrom · 2016-01-20T18:56:46Z

Review status: 6 of 24 files reviewed at latest revision, 6 unresolved discussions.

components/gfx/Cargo.toml, line 88 [r4] (raw file):
Done.

components/servo/Cargo.lock, line 72 [r4] (raw file):
Done.

components/servo/Cargo.lock, line 913 [r4] (raw file):
Done.

components/servo/Cargo.lock, line 1660 [r4] (raw file):
Done.

components/util/Cargo.toml, line 71 [r4] (raw file):
Done.

ports/gonk/Cargo.lock, line 54 [r4] (raw file):
Done.

Comments from the review on Reviewable.io

wafflespeanut · 2016-01-20T19:06:46Z

I've looked into some of the Python code.

Review status: 6 of 25 files reviewed at latest revision, 9 unresolved discussions.

python/servo/command_base.py, line 21 [r4] (raw file):
This could be

BIN_SUFFIX = '.exe' if sys.platform == 'win32' else ''

python/servo/command_base.py, line 46 [r4] (raw file):
We could really go for something like

known_os = {
'linux': 'unknown-linux-gnu',
# all other OSes
}
os_type = 'pc-windows-gnu' if os_type.startswith('mingw64_nt-') else known_os.get(os_type, 'unknown')

python/servo/command_base.py, line 65 [r4] (raw file):
comprehension (we don't really need bools):

return int(os.environ.get('SERVO_USE_NIGHTLY_RUST', 0))

python/servo/command_base.py, line 77 [r4] (raw file):
could be just

return subprocess.call(*args, shell = sys.platform == 'win32', **kwargs)

python/servo/command_base.py, line 89 [r4] (raw file):
Instead of having two functions which share the same code, we could go for a single function which takes one more key/value pair in kwargs (say, call_func), so that we can pass whatever object we want - something like,

def verbose_caller(*args, **kwargs):
    # verbose stuff ...
    call_func = kwargs.pop('call_func', subprocess.call)
    # default to subprocess.call (we could also pass subprocess.check_call to `kwargs`)
    return call_func(*args, shell = sys.platform == 'win32', **kwargs)

Comments from the review on Reviewable.io

jdm · 2016-01-20T19:09:32Z

Reviewed 6 of 6 files at r5.
Review status: 12 of 25 files reviewed at latest revision, 5 unresolved discussions.

Comments from the review on Reviewable.io

frewsxcv · 2016-01-20T19:11:09Z

components/util/workqueue.rs, line 108 [r5] (raw file):
Is Sleep(0) the same as not calling it at all?

Comments from the review on Reviewable.io

frewsxcv · 2016-01-20T19:12:33Z

Review status: 12 of 25 files reviewed at latest revision, 7 unresolved discussions.

components/compositing/constellation.rs, line 413 [r5] (raw file):
Inconsistent indentation

python/servo/command_base.py, line 65 [r4] (raw file):
Why should we use integers instead of booleans?

Comments from the review on Reviewable.io

frewsxcv · 2016-01-20T19:13:35Z

python/servo/command_base.py, line 77 [r4] (raw file):
Not the same. shell could be True on Linux and your suggestion won't reflect that.

Comments from the review on Reviewable.io

frewsxcv · 2016-01-20T19:16:23Z

Review status: 12 of 25 files reviewed at latest revision, 8 unresolved discussions.

python/servo/command_base.py, line 227 [r5] (raw file):
I think type(env['PATH']) == unicode might be the same as isinstance(env['PATH'], unicode), though I'm not sure of the functional differences

Comments from the review on Reviewable.io

wafflespeanut · 2016-01-20T19:21:07Z

Review status: 12 of 25 files reviewed at latest revision, 8 unresolved discussions.

python/servo/command_base.py, line 65 [r4] (raw file):
Well, because we can eliminate one other comparison, and it's the Pythonic way. When we only want to check whether the object is something, why not pass the object itself?

python/servo/command_base.py, line 77 [r4] (raw file):
The docs seem to say that shell = True is a security hazard and it's False by default.

Comments from the review on Reviewable.io

frewsxcv · 2016-01-20T19:26:28Z

python/servo/command_base.py, line 65 [r4] (raw file):
If a function should return "yes" or "no", it should return a boolean, not an integer. Right?

This is how I would write it personally:


def use_nightly_rust():
    envvar = os.environ.get("SERVO_USE_NIGHTLY_RUST", "0")
    return envvar != "0"

Comments from the review on Reviewable.io

emilio · 2016-01-20T20:20:48Z

Review status: 12 of 25 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

components/servo/lib.rs, line 270 [r5] (raw file):
nit: Indentation

components/servo/lib.rs, line 275 [r5] (raw file):
Here too

Comments from the review on Reviewable.io

larsbergstrom · 2016-01-21T14:47:21Z

Review status: 12 of 25 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

components/compositing/constellation.rs, line 413 [r5] (raw file):
Done.

components/servo/lib.rs, line 270 [r5] (raw file):
Done.

components/servo/lib.rs, line 275 [r5] (raw file):
Done.

components/util/workqueue.rs, line 108 [r5] (raw file):
@vvuk, can you comment here?

python/servo/command_base.py, line 21 [r4] (raw file):
Done.

python/servo/command_base.py, line 46 [r4] (raw file):
Can we do this as a follow-up E-Easy? I'm not confident in my python-fu to not mess this up on a less supported platform :-)

python/servo/command_base.py, line 65 [r4] (raw file):
Done.

python/servo/command_base.py, line 77 [r4] (raw file):
I'm not sure what to do here, given this comment thread?

python/servo/command_base.py, line 89 [r4] (raw file):
Another E-Easy follow-up, if you don't mind :-)

Comments from the review on Reviewable.io

larsbergstrom · 2016-01-21T14:55:43Z

@pcwalton can you please look at larsbergstrom@843f983?
(ignore the switch on the gaol repo; it's switched back to yours in a later cargo commit- I will do some squashing)

vvuk · 2016-01-21T15:15:27Z

Review status: 12 of 25 files reviewed at latest revision, 9 unresolved discussions, some commit checks failed.

components/util/workqueue.rs, line 108 [r5] (raw file):
No -- Sleep(0) gives any other threads a chance to run if any are ready to do so.

python/servo/command_base.py, line 46 [r4] (raw file):
Obviously up to you guys, but the proposed alternative looks ugly as hell to me... the if/else tree uses a lot of vertical space, but it's pretty clear what it's doing. If you want to turn it into a table lookup, then I'd suggest using a dict with regexps so that everything can be kept in it instead of having one-offs for windows.

Comments from the review on Reviewable.io

frewsxcv · 2016-01-21T15:19:00Z

python/servo/command_base.py, line 46 [r4] (raw file):
Personally, I prefer the conditional over dictionary indexing. The latter doesn't really make any sense to me in this case

Comments from the review on Reviewable.io

emilio · 2016-01-21T16:14:44Z

Tidy says the following:

./components/compositing/constellation.rs:453: trailing whitespace
./python/servo/bootstrap_commands.py:31: F401 'use_nightly_rust' imported but unused
./python/servo/command_base.py:21: E302 expected 2 blank lines, found 1
./python/servo/devenv_commands.py:99: F821 undefined name 'subprocess'

And this fails to build in travis due to:

/home/travis/build/servo/servo/components/compositing/lib.rs:29:1: 29:19 error: can't find crate for `gaol` [E0463]
/home/travis/build/servo/servo/components/compositing/lib.rs:29 extern crate gaol;
                                                                ^~~~~~~~~~~~~~~~~~
error: aborting due to previous error
Build failed, waiting for other jobs to finish...
Could not compile `compositing`.

wafflespeanut · 2016-01-22T02:21:39Z

Review status: 9 of 25 files reviewed at latest revision, 8 unresolved discussions.

python/servo/command_base.py, line 77 [r4] (raw file):
It could be what I suggested if @frewsxcv is okay with it...

python/servo/command_base.py, line 89 [r4] (raw file):
Yes, we need E-easy!

Comments from the review on Reviewable.io

frewsxcv · 2016-01-22T02:28:03Z

python/servo/command_base.py, line 77 [r4] (raw file):
I think @wafflespeanut's suggestion is fine, though make sure to format it like

return subprocess.call(*args, shell=sys.platform == 'win32', **kwargs)

# or

shell= sys.platform == 'win32'
return subprocess.call(*args, shell=shell, **kwargs)

or the style checker (flake8) will complain (you could add parens for clarity if you wish). Make sure to retain the comment about the PATH handling on windows.

Comments from the review on Reviewable.io

frewsxcv · 2016-01-22T02:28:36Z

python/servo/command_base.py, line 77 [r4] (raw file):
I meant:

shell = sys.platform == 'win32'

for line 5 of my previous post

Comments from the review on Reviewable.io

emilio · 2016-01-22T03:42:51Z

Review status: 9 of 25 files reviewed at latest revision, 8 unresolved discussions.

components/util/workqueue.rs, line 108 [r5] (raw file):
Oh, btw, this could use std::thread::yield_now, but I don't know if it's really worth it since in this case this is Win32 specific (we'll get rid of the kernel32 dep though).

I also don't understand why this ignores the usec param (will this potentially spin up the cpu all the time?), but I assume that has been already thought (I think I read in IRC this was left as a followup).

Comments from the review on Reviewable.io

pcwalton · 2016-01-22T22:43:40Z

@larsbergstrom That commit looks good to me.

larsbergstrom · 2016-01-23T01:05:14Z

Review status: 9 of 25 files reviewed at latest revision, 8 unresolved discussions.

components/util/workqueue.rs, line 108 [r5] (raw file):
Yes, we will follow up on this with an appropriate MsgWaitForMultipleObjectsEx (sleep on main thread is bad on windows because it will prevent the window from repainting during a drag because the message loop doesn't pump).

python/servo/command_base.py, line 46 [r4] (raw file):
Done.

python/servo/command_base.py, line 77 [r4] (raw file):
Done.

python/servo/command_base.py, line 89 [r4] (raw file):
Done.

Comments from the review on Reviewable.io


        adding check for windows then using Scripts instead of bin


        Remove usage of Gaol in the Windows configuration, since sandboxing i…

…s not supported there yet.


        Add Windows support for handle_event


        Cargo.lock updates

frewsxcv · 2016-01-23T01:39:03Z

Python stuff LGTM

larsbergstrom · 2016-01-23T01:57:23Z

@bors-servo r=frewsxcv,pcwalton,jdm,ecoal95

bors-servo · 2016-01-23T01:57:24Z

📌 Commit e9d9d1d has been approved by frewsxcv,pcwalton,jdm,ecoal95

bors-servo · 2016-01-23T01:57:27Z

⌛ Testing commit e9d9d1d with merge 525e77f...


        Auto merge of #9385 - larsbergstrom:win32, r=frewsxcv,pcwalton,jdm,ec…

@frewsxcv

…oal95 Win32 support r? @frewsxcv for python stuff r? @pcwalton for the "remove usage of Gaol" stuff for Win32 r? anybody else for misc cargo.lock updates, etc. This replaces #7878. This works best with servo/mozjs#71, too, to enable static linking, but can be run without (via some PATH hackery). The instructions are here, and will be added to a .md file in the repo once the mozjs changes also land: https://hackpad.com/Servo-on-Windows-C1LPcI2bP25 I'd like to get these changes landed because I've been rebasing them for months, they're otherwise quite stable, and don't affect our other platforms and targets.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9385)

bors-servo · 2016-01-23T02:47:03Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

frewsxcv · 2016-01-23T02:48:43Z

🎊

paulrouget · 2016-01-23T04:53:45Z

@larsbergstrom Should we move https://hackpad.com/Servo-on-Windows-C1LPcI2bP25 to servo/wiki?

larsbergstrom · 2016-01-23T17:09:06Z

@paulrouget I meant to move it to Readme.md, and will do so in a follow-up. Thanks for the reminder!

ghost · 2016-01-27T18:36:10Z

Great! 👍
Could we also have AppVeyor CI configured (http://www.appveyor.com/docs/appveyor-yml) to see Windows builds on CI via MinGW and Visual Studio? e.g. https://github.com/Snaipe/Criterion/blob/bleeding/appveyor.yml

frewsxcv · 2016-01-27T18:37:48Z

@jasonwilliams200k #9406

vvuk and others added 6 commits Oct 5, 2015

win32: use fontconfig/freetype on windows as well (for now)

025ed57

win32: make sleep_microseconds for backoff just do a win32 Sleep(0)

5e136d6

win32: look for python.exe and variants on win32 in style/build.rs

77aea59

win32: force PATH to be non-unicode on win32, if virtualenv munges it

5b845cb

Cargo updates and small workqueue change

095658e

highfive added the S-awaiting-review label Jan 20, 2016

larsbergstrom mentioned this pull request Jan 20, 2016

Get servo working on win32 #7878

Closed

jdm added the S-fails-tidy label Jan 21, 2016

frewsxcv removed the S-fails-tidy label Jan 22, 2016

jasonwilliams and others added 4 commits Jan 5, 2016

adding check for windows then using Scripts instead of bin

13d98f1

Remove usage of Gaol in the Windows configuration, since sandboxing i…

721693c

…s not supported there yet.

Add Windows support for handle_event

c655989

Cargo.lock updates

Loading status checks…

e9d9d1d

larsbergstrom force-pushed the larsbergstrom:win32 branch from 671e587 to e9d9d1d Jan 23, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 23, 2016

bors-servo merged commit e9d9d1d into servo:master Jan 23, 2016
2 checks passed

2 checks passed

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

homu Test successful
Details

This was referenced Jan 27, 2016

Servo over Xming is slow, unreliable #1769

Closed

Get Servo working on Windows #1908

Closed

larsbergstrom deleted the larsbergstrom:win32 branch Sep 29, 2016

servo / servo

Win32 support #9385

Win32 support #9385

larsbergstrom commented Jan 20, 2016

highfive commented Jan 20, 2016

highfive commented Jan 20, 2016

jdm commented Jan 20, 2016

larsbergstrom commented Jan 20, 2016

wafflespeanut commented Jan 20, 2016

jdm commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

wafflespeanut commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

emilio commented Jan 20, 2016

larsbergstrom commented Jan 21, 2016

larsbergstrom commented Jan 21, 2016

vvuk commented Jan 21, 2016

frewsxcv commented Jan 21, 2016

emilio commented Jan 21, 2016

wafflespeanut commented Jan 22, 2016

frewsxcv commented Jan 22, 2016

frewsxcv commented Jan 22, 2016

emilio commented Jan 22, 2016

pcwalton commented Jan 22, 2016

larsbergstrom commented Jan 23, 2016

frewsxcv commented Jan 23, 2016

larsbergstrom commented Jan 23, 2016

bors-servo commented Jan 23, 2016

bors-servo commented Jan 23, 2016

bors-servo commented Jan 23, 2016

frewsxcv commented Jan 23, 2016

paulrouget commented Jan 23, 2016

larsbergstrom commented Jan 23, 2016

ghost commented Jan 27, 2016

frewsxcv commented Jan 27, 2016

servo / servo

Join GitHub today

Win32 support #9385

Win32 support #9385

Conversation

larsbergstrom commented Jan 20, 2016

highfive commented Jan 20, 2016

highfive commented Jan 20, 2016

jdm commented Jan 20, 2016

larsbergstrom commented Jan 20, 2016

wafflespeanut commented Jan 20, 2016

jdm commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

wafflespeanut commented Jan 20, 2016

frewsxcv commented Jan 20, 2016

emilio commented Jan 20, 2016

larsbergstrom commented Jan 21, 2016

larsbergstrom commented Jan 21, 2016

vvuk commented Jan 21, 2016

frewsxcv commented Jan 21, 2016

emilio commented Jan 21, 2016

wafflespeanut commented Jan 22, 2016

frewsxcv commented Jan 22, 2016

frewsxcv commented Jan 22, 2016

emilio commented Jan 22, 2016

pcwalton commented Jan 22, 2016

larsbergstrom commented Jan 23, 2016

frewsxcv commented Jan 23, 2016

larsbergstrom commented Jan 23, 2016

bors-servo commented Jan 23, 2016

bors-servo commented Jan 23, 2016

bors-servo commented Jan 23, 2016

frewsxcv commented Jan 23, 2016

paulrouget commented Jan 23, 2016

larsbergstrom commented Jan 23, 2016

ghost commented Jan 27, 2016

frewsxcv commented Jan 27, 2016