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

tracking issue: full VS2017 support #13052

Closed
8 of 10 tasks
refack opened this issue May 16, 2017 · 12 comments
Closed
8 of 10 tasks

tracking issue: full VS2017 support #13052

refack opened this issue May 16, 2017 · 12 comments
Labels
build Issues and PRs related to build files or the CI. tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform.

Comments

@refack
Copy link
Contributor

refack commented May 16, 2017

  • Version: *
  • Platform: Windows
  • Subsystem: *

Tracking issue

Goal: get node compiling with VS2017 and passing all tests

breakdown of CI task by @joaocgreis:

@refack refack self-assigned this May 16, 2017
@refack refack added build Issues and PRs related to build files or the CI. lts-agenda tools Issues and PRs related to the tools directory. windows Issues and PRs related to the Windows platform. labels May 16, 2017
@refack
Copy link
Contributor Author

refack commented May 16, 2017

/cc @nodejs/lts is it a goal for porting? (re: #12450 (comment) and following comments)

@joaocgreis
Copy link
Member

create CI target

I'm going to move forward with that.

pass all tests

I ran the tests with master when we started adding support and everything passed. Didn't try recently though. Re #12450 (comment), can you paste the errors you get here?

@refack
Copy link
Contributor Author

refack commented May 16, 2017

I guess there's something wonky with symlinks on my machine.

parallel\test-module-circular-symlinks
Path: parallel/test-module-circular-symlinks
assert.js:92
  throw new AssertionError({
  ^

AssertionError [ERR_ASSERTION]: false == true
    at Object.<anonymous> (d:\code\node-cur\test\parallel\test-module-circular-symlinks.js:68:8)
    at Module._compile (module.js:569:30)
    at Object.Module._extensions..js (module.js:580:10)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Function.Module.runMain (module.js:605:10)
    at startup (bootstrap_node.js:144:16)
    at bootstrap_node.js:561:3
Command: d:\code\node-cur\Release\node.exe d:\code\node-cur\test\parallel\test-module-circular-symlinks.js
sequential\test-benchmark-http
...

http\http_server_for_chunky_client.js
http\_chunky_http_client.js type="send" n=1 len=1: 105.496705390639

http\simple.js
Command: d:\code\node-cur\Release\node.exe d:\code\node-cur\test\sequential\test-benchmark-http.js
--- TIMEOUT ---
addons/symlinked-module/test
Path: addons/symlinked-module/test
module.js:598
  return process.dlopen(module, path._makeLong(filename));
                 ^

Error: Module did not self-register.
    at Object.Module._extensions..node (module.js:598:18)
    at Module.load (module.js:503:32)
    at tryModuleLoad (module.js:466:12)
    at Function.Module._load (module.js:458:3)
    at Module.require (module.js:513:17)
    at require (internal/module.js:11:18)
    at __dirname.forEach (d:\code\node-cur\test\addons\symlinked-module\test.js:30:15)
    at Array.forEach (native)
    at Object.<anonymous> (d:\code\node-cur\test\addons\symlinked-module\test.js:29:24)
    at Module._compile (module.js:569:30)
Command: d:\code\node-cur\Release\node.exe d:\code\node-cur\test\addons\symlinked-module\test.js

@joaocgreis
Copy link
Member

joaocgreis commented May 23, 2017

Added VS2017 to node-test-commit-windows-fanned, but only for Node >= version 8 and onlly for the compiling stage (i.e., not for testing addons, that is blocked until npm is updated with a recent version of node-gyp).

Current status:

@refack
Copy link
Contributor Author

refack commented May 23, 2017

Added VS2017 to node-test-commit-windows-fanned

@joaocgreis 👍
I copied your list into the first comment.

@sam-github
Copy link
Contributor

What is the specific question that is on the LTS agenda for this issue?

@refack
Copy link
Contributor Author

refack commented Sep 19, 2017

Do we want to start building v9.0.0 in VS2017 (or even move v8.x to VS2017 for LTS)?

@sam-github
Copy link
Contributor

sam-github commented Sep 19, 2017

If it has no ABI or platform support implications then @nodejs/lts doesn't have an opinion.

@refack refack removed the lts-agenda label Sep 19, 2017
@refack
Copy link
Contributor Author

refack commented Sep 20, 2017

@joaocgreis what do you think about building v8.6.0 with vs2017? So we can see if it's good, and move to use it for v8.x as it goes into LTS?

/cc @nodejs/platform-windows

@joaocgreis
Copy link
Member

Node v8 and v9 are far too close to move now, there are things to do that will take time.

  • First, we have to carefully check native modules. CitGM automates part of this, but the CI job is only running in VS2015 and for this we'd have to compile with VS2017 and run CitGM with other versions, because ABI compatibility is not guaranteed (it is for C, but not for C++, and has been broken in the past). We can do this locally but sooner or later it'd be good to add to the CI job.
  • Then, we'll have to create the release machines and change the release job. This should be easy.
  • And finally we should release nightlies for at least a few months, to have a good chance of finding issues in the wild.

To be careful, I'd wait a lot more before moving. But on the other hand we'll have to maintain the VS2015 infrastructure until it's needed, and sooner or later we'll start to have issues caused by VS2015. @refack are there other compelling reasons to move?

So, my recommendation is that we move to VS2017 for v10. We can start the steps above after v9 is released to start releasing nightlies of v10 early, if no technical issues are found. Note that VS2015 is still in wide use, so we'll still have to support building node and native modules with it, as v10 is still too early to drop that.

Ref issues where we dropped VS2013 and moved to VS2015 (as opposed to here, where we're just moving to VS2017, not dropping VS2015): #7484, #7989.

cc @nodejs/build

@gibfahn
Copy link
Member

gibfahn commented Sep 22, 2017

Sounds to me like all the steps we can take to prepare for using VS2017 can be started now without considering which version of Node it should land in. Once we're confident and have a few months of nightlies, we can switch for whichever is the next version of Node at the time.

In fact if it's ABI compatible we can switch inside a release right? We'll have to test with all versions anyway.

Agreed that Node 10.x sounds too early for dropping VS2015.

@joaocgreis
Copy link
Member

In fact if it's ABI compatible we can switch inside a release right?

We don't have guarantees of ABI compatibility. We should search for problems and hopefully find none, but we don't know for sure with all the exposed C++ ABI. So, it would be better to do it in a major version.

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

No branches or pull requests

4 participants