search

servo / rust-mozjs

DEPRECATED - moved to servo/mozjs instead.
Mozilla Public License 2.0
293 stars 122 forks source link

[msvc] Support MSVC 12/14 bindings generation and builds #270

Closed vvuk closed 8 years ago

vvuk commented 8 years ago

This change is Reviewable

nox commented 8 years ago

There should be _debug versions of the bindings too.

upsuper commented 8 years ago

FWIW, it seems to me you may want to add "-D_CRT_USE_BUILTIN_OFFSETOF" to compiler parameters, otherwise it may complain on reinterpret_cast in static_assert.

vvuk commented 8 years ago

Oh hey, that would let me remove some of the #ifdefs in mozjs; didn't realize that's what causes it. Thanks!

metajack commented 8 years ago

@nox should review the bindgen stuff. The makefile stuff looks fine to me. What is the 12? VS2012? Can we not just use 2014 or whatever the latest is and not worry about older ones?

nox commented 8 years ago

This looks ok to me.

bors-servo commented 8 years ago

:umbrella: The latest upstream changes (presumably #278) made this pull request unmergeable. Please resolve the merge conflicts.

bors-servo commented 8 years ago

:umbrella: The latest upstream changes (presumably #280) made this pull request unmergeable. Please resolve the merge conflicts.

Ms2ger commented 8 years ago

Upstream only supports Visual C++ 2015 Update 2 or newer; should we match that?

metajack commented 8 years ago

That seems logical.

Ms2ger commented 8 years ago

Please also apply

diff --git a/appveyor.yml b/appveyor.yml
index e1aa757..279da86 100644
--- a/appveyor.yml
+++ b/appveyor.yml
@@ -4,10 +4,9 @@ environment:
   PATH: 'C:\msys64\mingw64\bin;C:\msys64\usr\bin\;%PATH%'
   MSYSTEM: 'MSYS'
   MSYS: 'winsymlinks=lnk'
-  TARGET: 'nightly-x86_64-pc-windows-gnu'
-
-platform:
-  - x64
+  matrix:
+  - TARGET: nightly-x86_64-pc-windows-msvc
+  - TARGET: nightly-x86_64-pc-windows-gnu

 install:
   - bash -lc "echo $MSYSTEM; pacman --needed --noconfirm -Sy pacman-mirrors"
vvuk commented 8 years ago

Upstream only supports Visual C++ 2015 Update 2 or newer; should we match that?

Yeah, I guess I kept the msvc12 code in there solely because I already did it, and someone might already want to do it.. but it's a good point that upstream requires 2015, I thought we still supported 2013.

vvuk commented 8 years ago
  • - TARGET: nightly-x86_64-pc-windows-msvc

I don't think this will work -- we will actually need to install the VS2015 build tools in appveyor to do this.

I would suggest that we centralize the "windows machine setup" into a separate repo that we download a bash script for and pass $TARGET to, instead of a bunch of bash commands.. that way it can be shared amongst all the repos that want to do windows testing, since they'll all want a similar build environment.

vvuk commented 8 years ago

That's quite the failure on msvc. It builds locally so I suspect I just have a bad merge or bad version selection somewhere. Will fix tomorrow.

vvuk commented 8 years ago

Ok, not sure what's going on here. This builds locally with the exact same commit of mozjs_sys. I've listed the failing (from appveyor) and the working (from local) link lines in this gist: https://gist.github.com/vvuk/a647def60b4a6493f299100b1ff73afe

I don't really see anything different between the two. Only thing I can think of is if somehow stuff isn't making it into libmozjssys-.rlib. On my local build, libmozjssys-.rlib is ~130MB. Can we find out what the size is on the appveyor builders?

larsbergstrom commented 8 years ago

Looking into it now!

BTW, there is a nifty hack for AppVeyor to get RDP access to the builders here: https://www.appveyor.com/docs/how-to/rdp-to-build-worker

I use that pretty frequently to debug AppVeyor shenanigans.

larsbergstrom commented 8 years ago

Here's the result:

C:\windows\system32>dir C:\projects\rust-mozjs\target\debug\deps\libmozjs_sys-57
2528194a83d174.rlib
 Volume in drive C has no label.
 Volume Serial Number is 1ACC-0658

 Directory of C:\projects\rust-mozjs\target\debug\deps

08/08/2016  12:21 AM       320,754,338 libmozjs_sys-572528194a83d174.rlib
               1 File(s)    320,754,338 bytes
               0 Dir(s)  25,959,452,672 bytes free

C:\windows\system32>
metajack commented 8 years ago

What's a 2.5x difference among friends?

larsbergstrom commented 8 years ago

I'm curious as to whether these are related to the specific Rust compiler version being used? They did a lot of recent refactoring to rustc and just turned on the new intermediate representation. Perhaps that is changing symbol visibility?

@alexcrichton might know more if there were any recent changes to symbol visibility on Windows.

Otherwise, the only time I see this issue is when the link order is wrong, but it seems right in this case.

alexcrichton commented 8 years ago

AFAIK there haven't been any changes in symbol visibility on Windows recently, or at least if there were it's an unknown regression. I also think that link.exe on Windows doesn't worry about order on the command line, so that's probably not the cause here either :(.

I wonder if different MSVC versions are being used on appveyor or perhaps a library is forgetting to get linked?

larsbergstrom commented 8 years ago

@bors-servo r+

This looks awesome - great work! Listing out the PATH is pretty similar to what we have to do on the buildbot side of things, too. imo, it's better to do that than rely on hoping that the machine & environment are appropriately provisioned/configured, especially right now while we don't have reliable salt/chef/ansible/whatever provisioning our Windows builders.

bors-servo commented 8 years ago

:pushpin: Commit 32bc9c6 has been approved by larsbergstrom

bors-servo commented 8 years ago

:hourglass: Testing commit 32bc9c6 with merge 36a3ec0...

jdm commented 8 years ago

What happened here? Did homu get stuck?

jdm commented 8 years ago

Oh, is it the missing travis integration?

larsbergstrom commented 8 years ago

Yup! Passed: https://travis-ci.org/servo/rust-mozjs/builds/150968226