Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VS 2015 support #478

Closed
wants to merge 315 commits into from
Closed

VS 2015 support #478

wants to merge 315 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 17, 2015

Not quite working yet, it gets to the openssl-cli compile and fails with a linking error:

apps.obj : error LNK2001: unresolved external symbol ___iob_func [C:\io.js\deps\openssl\openssl-cli.vcxproj]
C:\io.js\Release\openssl-cli.exe : fatal error LNK1120: 1 unresolved externals [C:\io.js\deps\openssl\openssl-cli.vcxproj]

This PR upgrades GYP to the latest which understands 2015, and has a lot more changes because there hasn't been an upgrade in a while I suppose.

I've also edited vcbuild.bat to do the appropriate checks.

@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/85/

which includes a VS2015 host

@rvagg rvagg force-pushed the msvs-2015 branch 2 times, most recently from 03614ae to 9e8f118 Compare January 17, 2015 07:47
@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

new build: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/88/

I don't understand the Python stack trace for the OSX failures, anybody care to try and enlighten me on that one?

@chrisdickinson
Copy link
Contributor

Re: python: something in here is adding a non-string-y item to the env dict (likely it's adding None), and re.sub is exploding when it's handed that value.

@bnoordhuis
Copy link
Member

@rvagg Can you mention the gyp commit (e.g. r1234) it's been upgraded to? I'll see if I can reproduce the OS X failure on my 10.8 machine.

EDIT: No, seems to be working for me.

@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

@bnoordhuis I would have put a version in there if I knew where to find one, I went looking but alas.. Any hints on where to find a version for this?

@bnoordhuis
Copy link
Member

@rvagg I don't know where you got it from but if it's from subversion, then it's the svn commit. If it's git, it's the number from git-svn-id. If this upgrade is to today's HEAD, that would make it r2024.

@chrisdickinson The stack trace suggests it's from here but it looks to me that additional_settings[k] has been sanitized at that point. Strange.

@chrisdickinson
Copy link
Contributor

@bnoordhuis I thought so too at first, but additional_settings gets updated with the contents of env.

@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

I printed them out and it is failing on a None, SDKROOT is the one it's borking on.

@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

note this machine only has the commandline tools, not full xcode, which I'm guessing is partly to blame here, still a bug though

@rvagg rvagg force-pushed the msvs-2015 branch 3 times, most recently from 54f093d to 21dd523 Compare January 17, 2015 12:57
@rvagg
Copy link
Member Author

rvagg commented Jan 17, 2015

figured it out, needs a new entry in common.gypi:

'SDKROOT': 'macosx'

building here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/90/

@bnoordhuis
Copy link
Member

LGTM. Maybe you can move the change to common.gypi into a separate commit with an explanation of why it's needed (and move it before the gyp upgrade to keep git bisect happy.)

The iojs-win2012r2-msvs2015 failure seems to be related to this. I'm not sure why OpenSSL does that but I think we can just rip it out, the only part of OpenSSL that uses stdio (in our configuration) are the apps.

@piscisaureus or @seishun Agree/disagree?

@rvagg
Copy link
Member Author

rvagg commented Jan 19, 2015

Good guess @bnoordhuis, when I fix the define to exclude MSVS 2015 it compiles fine and tests pass with only 1 failure (a TLS failure mind you...). See 7b66043.

Re-running tests in CI here: https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/94/nodes=iojs-win2012r2-msvs2015/console

@rvagg
Copy link
Member Author

rvagg commented Jan 19, 2015

oh, that was an all-pass that time, neat.

@bnoordhuis
Copy link
Member

@rvagg It looks like it's running the message tests only.

@rvagg
Copy link
Member Author

rvagg commented Jan 19, 2015

@rvagg rvagg closed this Jan 19, 2015
@rvagg rvagg reopened this Jan 19, 2015
@rvagg
Copy link
Member Author

rvagg commented Jan 19, 2015

Ugh, github on phone, I shouldn't try and be productive. Will look in to this tomorrow.

@seishun
Copy link
Contributor

seishun commented Jan 20, 2015

I don't like the idea of floating patches to OpenSSL to support a preview version of Visual Studio. Maybe it's better to wait for OpenSSL to fix it on their end?

@rvagg
Copy link
Member Author

rvagg commented Jan 20, 2015

Yes, point taken. I was surprised that it wasn't already fixed tbh. I'll leave this PR open so ppl know we are tracking towards it but waiting for it to be easier.

am11 referenced this pull request in microsoft/openssl Jan 29, 2015
Fixes builds where environment variable has backslashes
@xforce
Copy link

xforce commented Feb 4, 2015

It is fixed in OpenSSL 1.0.2. Patch is from 2 July 2014 http://git.openssl.org/gitweb/?p=openssl.git;a=commitdiff;h=aab3560b65b9254d17770bb6fe3ca7edd7451429

@Fishrock123
Copy link
Contributor

OpenSSL 1.0.2 discussion: #589

jbergstroem and others added 12 commits June 17, 2015 10:07
Make the order of name, email and other additions simpler to
copy paste and/or match with git commit messages.

Useful when working with `Reviewed-By`.

PR-URL: nodejs#1966
Reviewed-By: Christian Tellnes <christian@tellnes.no>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
tmpdir creation only happens for tests that need it. So failure to
refresh the temporary directory should result in a failed test.

PR-URL: nodejs#1976
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Futher discussion at nodejs/node-v0.x-archive#25294

PR-URL: nodejs#1926
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
The accessors run inside an implicit HandleScope, there is no need to
create a new one.

PR-URL: nodejs#2001
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Re-add the wrapper class id to AsyncWrap instances so they can be
tracked directly in a heapdump.

Previously the class id was given without setting the heap dump wrapper
class info provider. Causing a segfault when a heapdump was taken. This
has been added, and the label_ set to the given provider name so each
instance can be identified.

The id will not be set of the passed object has no internal field count.
As the class pointer cannot be retrieved from the object.

In order to properly report the allocated size of each class, the new
pure virtual method self_size() has been introduces.

PR-URL: nodejs#1896
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: nodejs#1989
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
PR-URL: nodejs#1943
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: nodejs#1994
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Defer reading until user-land has a chance to add listeners. This
allows the TLS wrapper to listen for _tlsError and trigger a
clientError event if the socket already has data that could trigger.

Fixes: nodejs#1114
PR-URL: nodejs#1496
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
require() now checks that the path exists before searching
further in it.

PR-URL: nodejs#1920
Reviewed-By: Isaac Z. Schlueter <i@izs.me>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Add link to Jenkins server in Collaborator Guide.

PR-URL: nodejs#1995
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
PR-URL: nodejs#2000
Reviewed-By: Yosuke Furukawa <yosuke.furukawa@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@targos
Copy link
Member

targos commented Jun 18, 2015

rvagg@0e54add that works for me (VS 2015 RC on latest Windows 10 build).

But v8 cannot compile because of a bug in VS. There is a workaround since v4.4.42.

@ghost
Copy link

ghost commented Jun 18, 2015

@targos, I would not consider that as a bug since in the linked Connect bug report, both OP (David) and VC team member (Jonathan) are in agreement that there is no violation of standard.

Note that the other bug in RC is fixed in recent compiler revision (build 19.00.23008, June 10, 2015) http://webcompiler.cloudapp.net/; it compiles with this code:

struct User {
  void *operator new(size_t, unsigned);
  void operator delete(void *);
};

int main()
{
    auto q = new (0) User;
    return 0;
}

@targos
Copy link
Member

targos commented Jun 18, 2015

@jasonwilliams200OK I was indeed talking about that other bug. They say it's fixed for RTM but is there a way to get the latest version of the C++ compiler before the release?

@ghost
Copy link

ghost commented Jun 18, 2015

I think no (that web-compiler is emerged as part of a research project http://rise4fun.com/), but if I venture an educated guess, VS RTM will be available few weeks before Windows 10 release (July 29, 2015), which is about any day now. :)
They are probably also considering to make cl (+link+libs) releases independent of VS release lifecycle, which means we could have win32 (cli-only) server-core running in Docker with cl installed for build stuff.

orangemocha and others added 2 commits June 19, 2015 11:04
vcbuild.bat calls python configure before setting GYP_MSVS_VERSION,
so SelectVisualStudioVersion (tools\gyp\pylib\gyp\MSVSVersion.py)
defaults to 'auto' and selects VS 2005.

vcbuild sets the environment in the current shell, so this issue
would manifest itself only on the first invocation of the script
in any given shell windows.

Reviewed-By: Julien Gilli <jgilli@fastmail.fm>
PR-URL: nodejs/node-v0.x-archive#20109
@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

@rvagg
Copy link
Member Author

rvagg commented Jun 19, 2015

@orangemocha
Copy link
Contributor

Did something get messed up with this PR? GitHub shows 315 commits and 6686 files changed.

@rvagg
Copy link
Member Author

rvagg commented Jun 22, 2015

yeah, it's really old and targetting the v1.x branch, hence the superfluous commits. It's just the head one that matters.

@targos
Copy link
Member

targos commented Jun 22, 2015

that's because the PR targets nodejs:v1.x and it is not possible to change the target branch afterwards. Only the last 2 commits are relevant.

@rvagg rvagg closed this Jun 23, 2015
@rvagg rvagg mentioned this pull request Jun 23, 2015
@srl295
Copy link
Member

srl295 commented Jul 30, 2015

@rvagg Intl won't work under VS2015 because of the same breakage (ICU bug).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.