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

Last builds lack Windows installers and binaries #19

Closed
vsemozhetbyt opened this issue Oct 20, 2017 · 40 comments
Closed

Last builds lack Windows installers and binaries #19

vsemozhetbyt opened this issue Oct 20, 2017 · 40 comments

Comments

@targos
Copy link
Member

targos commented Oct 20, 2017

That's because the build is broken on Windows: https://ci.nodejs.org/job/node-test-commit-windows-fanned/12687/

@refack
Copy link

refack commented Oct 20, 2017

Also checking out

@targos
Copy link
Member

targos commented Oct 26, 2017

@vsemozhetbyt
Copy link
Author

Still lack.
cc @nodejs/platform-windows?

@seishun
Copy link

seishun commented Oct 31, 2017

CI looks fine, so it's up to @nodejs/build

@targos
Copy link
Member

targos commented Oct 31, 2017

CI does not look fine. Here is the last run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/12997/

@seishun
Copy link

seishun commented Oct 31, 2017

Ah, the dreaded parallel/test-async-wrap-uncaughtexception. See nodejs/node#16210

@targos
Copy link
Member

targos commented Oct 31, 2017

Not really. It doesn't even compile.

@refack
Copy link

refack commented Oct 31, 2017

It's that json bug. I have a patch for that. I wanted to upstream. I'll float here.

@targos
Copy link
Member

targos commented Oct 31, 2017

@refack. Thanks. If you have a working patch, feel free to push a commit on top of canary-base (in nodejs/node, not this repo). The daily job will pick it up tomorrow.

@seishun
Copy link

seishun commented Oct 31, 2017

I see here that the last few builds were successful though: https://ci.nodejs.org/job/node-compile-windows/

@targos
Copy link
Member

targos commented Oct 31, 2017

We only have one canary build per day (unless it's manually triggered). They share the same platform jobs with regular CI.

@bzoz
Copy link

bzoz commented Oct 31, 2017

nodejs/node@dfcaf28 fixed Windows compilation

@seishun
Copy link

seishun commented Nov 1, 2017

@bzoz we're talking about v8-canary, not master. Took me a while to realize it too.

@vsemozhetbyt
Copy link
Author

Ping)

@targos
Copy link
Member

targos commented Nov 7, 2017

@seishun
Copy link

seishun commented Nov 7, 2017

Confirmed locally, doesn't build with VS2015.

Does V8 have an official supported compilers list? Microsoft may or may not fix the internal compiler error, but they are definitely not going to add relaxed constexpr in VS2015. It's supported since VS2017.

@targos
Copy link
Member

targos commented Nov 7, 2017

/cc @nodejs/v8

@refack
Copy link

refack commented Nov 7, 2017

This is after nodejs/node@5366f80?
(that is 1a0afde here)

@targos
Copy link
Member

targos commented Nov 7, 2017

@refack yes

@refack
Copy link

refack commented Nov 7, 2017

Does V8 have an official supported compilers list? Microsoft may or may not fix the internal compiler error, but they are definitely not going to add relaxed constexpr in VS2015. It's supported since VS2017.

The build infra is ready with VS2017, and we can do the shift in a major. But this is strange that it broke so fast, I thought V8 moved to building on VS2017 just a month ago. IMHO we could open a bug for that.

@refack
Copy link

refack commented Nov 7, 2017

Breaking CL landed a day ago: https://chromium-review.googlesource.com/c/v8/v8/+/753903

@hashseed
Copy link
Member

hashseed commented Nov 7, 2017

Thanks for pinpointing the culprit. We currently don't have a Windows bot testing Node.js integration on our end, but it looks like we need one. I'll try to get something set up soon.

@refack
Copy link

refack commented Nov 7, 2017

@hashseed even just compiling on VS2015 would be great help (we're stuck building with VS2015 for node 9, so six more months)

@backes
Copy link

backes commented Nov 8, 2017

@hashseed note that this is not about nodejs integration, V8 and chromium are currently not compiled at all with VS2015 AFAICT. So either V8 needs to return to supporting VS2015, or nodejs also switches to VS2017 as proposed in #16868. The latter seems to be the more attractive way forward to me.

@hashseed
Copy link
Member

hashseed commented Nov 8, 2017

What I'm saying is that as long as Node.js has not moved to VS2017, we should have a test bot to build Node.js with VS2015, which would have detected this problem.

It also means that V8 needs to support VS2015 until the switch.

@hashseed
Copy link
Member

hashseed commented Nov 8, 2017

Tracking bug here for those interested https://bugs.chromium.org/p/v8/issues/detail?id=5960

@joaocgreis
Copy link
Member

Do we need the version of V8 that is failing to land in node v9.x?

Adding a new release machine with VS2017 (ref nodejs/node#13052 and nodejs/node#16868) will fix this.

@hashseed I understand that maintaining VS2015 support is a burden for V8. However, for building native modules, node users must also have VS installed. Asking all node users with VS2015 to update is something that would be good to avoid, some of them might not even be able to. It would be great if V8 could test building node with VS2017 and then testing addons with VS2015, to avoid issues like #4. If this can be done, is there anything I can do to help?

@seishun
Copy link

seishun commented Nov 9, 2017

Asking all node users with VS2015 to update is something that would be good to avoid, some of them might not even be able to.

On the other hand, assuming VS2015 is binary incompatible with VS2017, we are currently forcing node users with VS2017 to install VS2015 alongside it, which is arguably worse.

@joaocgreis
Copy link
Member

we are currently forcing node users with VS2017 to install VS2015 alongside it

Are we? How? node-gyp should be able to use either and both are supported by us.

@refack
Copy link

refack commented Nov 9, 2017

Fix landed in V8 master, now being backported - https://bugs.chromium.org/p/v8/issues/detail?id=7061

@seishun
Copy link

seishun commented Nov 9, 2017

node-gyp should be able to use either and both are supported by us.

Well that confirms that VS2015 is binary compatible with VS2017. If they weren't, addons built with VS2017 wouldn't work with node built with VS2015.

@refack
Copy link

refack commented Nov 9, 2017

Ignore my last comment. It's about the build issue not the extended constexpr.

@refack
Copy link

refack commented Nov 9, 2017

There is the issue of building node core from source, not just addons. Dropping official support for VS2015 is one thing, actually breaking support it is semver-major.

Let's keep that discussion in nodejs/node#16868.

@targos
Copy link
Member

targos commented Nov 18, 2017

Do we have a release machine with VS2017?

@vsemozhetbyt vsemozhetbyt changed the title Last two builds lack Windows installers and binaries Last builds lack Windows installers and binaries Nov 18, 2017
@refack
Copy link

refack commented Nov 18, 2017

@joaocgreis is working on some bugs in vcbuild.bat -
nodejs/node#17101

@vsemozhetbyt
Copy link
Author

Is there any progress on this? We still have no Windows binaries.

@seishun
Copy link

seishun commented Nov 30, 2017

AFAIK @joaocgreis is working on a VS2017 release machine.

@joaocgreis
Copy link
Member

Release machines ready and working. First node-v8 test run: https://nodejs.org/download/test/v10.0.0-test201711308c5d0f1f35/ . Today's nightlies should already be build by the new machines, if everything goes well this should be fixed.

@vsemozhetbyt
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants