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.

Sign up for GitHub

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

Jump to bottom

src: refactor node options parsers to mitigate MSVC bug #26280

Merged
merged 2 commits into from
Mar 4, 2019
Merged

src: refactor node options parsers to mitigate MSVC bug #26280

merged 2 commits into from
Mar 4, 2019

Conversation

refack
Copy link
Contributor

@refack refack commented Feb 23, 2019

Simplify node_options API, so the compiler bug should not manifest.
Should fix current master
Fixes: #25593

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

@refack sadly an error occured when I tried to trigger a build :(

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 23, 2019
@refack
Copy link
Contributor Author

refack commented Feb 23, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/20988/

@Trott
Copy link
Member

Trott commented Feb 23, 2019

@nodejs/build @nodejs/platform-windows This is supposed to fix the current Windows CI problem.

@refack refack added the wip Issues and PRs that are still a work in progress. label Feb 23, 2019
@refack
Copy link
Contributor Author

refack commented Feb 23, 2019 •
edited
Loading

Still a WIP...
Now there's a problem with the x64 build 🤦‍♂️

@refack refack removed the wip Issues and PRs that are still a work in progress. label Feb 24, 2019
@refack
Copy link
Contributor Author

refack commented Feb 24, 2019 •
edited
Loading

Rebuild: https://ci.nodejs.org/job/node-test-pull-request/20989/
Rebuild: https://ci.nodejs.org/job/node-test-commit/26103/

@refack
Copy link
Contributor Author

refack commented Feb 24, 2019 •
edited
Loading

Resume: https://ci.nodejs.org/job/node-test-commit/26104/ (PI1 fail was infra)

@seishun
Copy link
Contributor

seishun commented Feb 24, 2019

Why is this necessary? Isn't #25593 already fixed?

@refack
Copy link
Contributor Author

refack commented Feb 24, 2019 •
edited
Loading

Why is this necessary? Isn't #25593 already fixed?

As you predicted the fix in #25596 was temporary, and the issue came back. The current code in master when compiled for ia32 results in a broken binary.

For example https://nodejs.org/download/nightly/v12.0.0-nightly201902247e0ddf66b9/win-x86/
image

Resume: https://ci.nodejs.org/job/node-test-commit/26108/

@addaleax
Copy link
Member

If the issue is still a static initialization order thing, It's not obvious to me how some of the changes here (e.g. to node_worker.cc, .begin() → std::begin(), header include order) relate to the issue at hand.

Other than that, the code changes here look good to me. It's unfortunate that the code has to be bloated up a bit, but it's clear that fixing a broken build is more important.

Do we know what change caused this? Should that have gotten a red CI? (Or is it still only debug builds?)

I also can't reproduce this locally on Windows on master, neither with Debug nor Release builds, but I can also only build for ia32 from x64, so maybe that's it?

@refack refack force-pushed the mitigate-msvc-compiler-issue branch from 6b14968 to 5cf7ba6 Compare February 25, 2019 02:22
@refack
Copy link
Contributor Author

refack commented Feb 25, 2019 •
edited
Loading

Reduced some bloat.
https://ci.nodejs.org/job/node-test-pull-request/20991/

joyeecheung
joyeecheung approved these changes Feb 25, 2019
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nits

src/node_options.cc Outdated Show resolved Hide resolved
src/node_options.h Outdated Show resolved Hide resolved
src/node_options.cc Outdated Show resolved Hide resolved
@refack refack force-pushed the mitigate-msvc-compiler-issue branch from dc38996 to f3a4e2b Compare February 25, 2019 16:44
@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 25, 2019
addaleax
addaleax reviewed Feb 25, 2019
View reviewed changes
src/node_options.cc Outdated Show resolved Hide resolved
src/node_worker.cc Outdated Show resolved Hide resolved
@refack refack self-assigned this Feb 25, 2019
@refack refack force-pushed the mitigate-msvc-compiler-issue branch from f3a4e2b to f98b845 Compare February 25, 2019 20:44
@refack
Copy link
Contributor Author

refack commented Feb 25, 2019

Rebased into two commits (1) bug fix and (2) fix warnings
CI: https://ci.nodejs.org/job/node-test-pull-request/20993/

@refack refack force-pushed the mitigate-msvc-compiler-issue branch 2 times, most recently from 251a80b to 57e2c03 Compare March 3, 2019 16:33
@refack
Copy link
Contributor Author

refack commented Mar 3, 2019

Fresh CI: https://ci.nodejs.org/job/node-test-pull-request/21167/

@refack refack force-pushed the mitigate-msvc-compiler-issue branch from 57e2c03 to 2c6d94f Compare March 4, 2019 01:03
@refack refack removed the request for review from bnoordhuis March 4, 2019 01:06
@refack refack removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 4, 2019
@refack refack merged commit 2c6d94f into nodejs:master Mar 4, 2019
@refack refack deleted the mitigate-msvc-compiler-issue branch March 4, 2019 01:06
BridgeAR pushed a commit that referenced this pull request Mar 4, 2019
@refack @BridgeAR
src: refactor node options parsers to mitigate MSVC bug
64a0b11 
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
BridgeAR pushed a commit that referenced this pull request Mar 4, 2019
@refack @BridgeAR
src: fix warnings around node_options
668ca36 
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Mar 4, 2019
Merged
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

I had to back out this PR from v11.11.0 due to #26322 (comment).

Should this be backported?

@refack refack removed their assignment Mar 11, 2019
@refack

This comment has been minimized.

Sign in to view
@refack
Copy link
Contributor Author

refack commented Mar 14, 2019

Backport PR: #26649

@refack refack self-assigned this Mar 14, 2019
@refack refack mentioned this pull request Mar 14, 2019
Closed
3 tasks
refack added a commit to refack/node that referenced this pull request Mar 14, 2019
@refack
src: refactor node options parsers to mitigate MSVC bug
4c02851 
PR-URL: nodejs#26280
Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
refack added a commit to refack/node that referenced this pull request Mar 14, 2019
@refack
src: fix warnings around node_options
1ad23f6 
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

PR-URL: nodejs#26280
Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 28, 2019
@refack @targos
src: refactor node options parsers to mitigate MSVC bug
62f9049 
Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 28, 2019
@refack @targos
src: fix warnings around node_options
361025f 
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
targos pushed a commit that referenced this pull request Mar 30, 2019
@refack @targos
src: fix warnings around node_options
e6c1ad5 
* header explicit usage, order, and reduce use of `*-inl.h`
* pointer -> const reference when possible
* no variable recyclicng
* `std::begin/end` prefered over `instance.begin/end`
* `USE` for explicit unused resaults

Backport-PR-URL: #26649
PR-URL: #26280
Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
Merged
@refack refack removed their assignment Apr 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@joyeecheung joyeecheung joyeecheung approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Debug build doesn't work on Windows
8 participants
@refack @nodejs-github-bot @Trott @seishun @addaleax @BridgeAR @joyeecheung @targos