Skip to content

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

Merged
refack merged 2 commits into
nodejs:masterfrom
refack:mitigate-msvc-compiler-issue
Mar 4, 2019
Merged

src: refactor node options parsers to mitigate MSVC bug#26280
refack merged 2 commits into
nodejs:masterfrom
refack:mitigate-msvc-compiler-issue

Conversation

@refack

@refackrefack commented Feb 23, 2019

Copy link
Copy Markdown
Contributor

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

nodejs-github-bot commented Feb 23, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-botnodejs-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

refack commented Feb 23, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@Trott

Trott commented Feb 23, 2019

Copy link
Copy Markdown
Member

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

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

refack commented Feb 23, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

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

refack commented Feb 24, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@refack

refack commented Feb 24, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

@seishun

seishun commented Feb 24, 2019

Copy link
Copy Markdown
Contributor

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

@refack

refack commented Feb 24, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

addaleax commented Feb 25, 2019

Copy link
Copy Markdown
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
refackforce-pushed the mitigate-msvc-compiler-issue branch from 6b14968 to 5cf7ba6CompareFebruary 25, 2019 02:22
@refack

refack commented Feb 25, 2019•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

joyeecheung
joyeecheung approved these changes Feb 25, 2019
View reviewed changes

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM with nits

Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_options.h Outdated
Comment threadsrc/node_options.cc Outdated
@refack
refackforce-pushed the mitigate-msvc-compiler-issue branch from dc38996 to f3a4e2bCompareFebruary 25, 2019 16:44
@TrottTrott 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
Comment threadsrc/node_options.cc Outdated
Comment threadsrc/node_worker.cc Outdated
@refackrefack self-assigned this Feb 25, 2019
@refack
refackforce-pushed the mitigate-msvc-compiler-issue branch from f3a4e2b to f98b845CompareFebruary 25, 2019 20:44
@refack

refack commented Feb 25, 2019

Copy link
Copy Markdown
ContributorAuthor

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

@refack
refackforce-pushed the mitigate-msvc-compiler-issue branch 2 times, most recently from 251a80b to 57e2c03CompareMarch 3, 2019 16:33
@refack

refack commented Mar 3, 2019

Copy link
Copy Markdown
ContributorAuthor

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

refack added 2 commits March 3, 2019 20:03
@refack
src: refactor node options parsers to mitigate MSVC bug
af8b92c
PR-URL: nodejs#26280Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack
src: fix warnings around node_options
2c6d94f
* 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#26280Fixes: nodejs#25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@refack
refackforce-pushed the mitigate-msvc-compiler-issue branch from 57e2c03 to 2c6d94fCompareMarch 4, 2019 01:03
@refack
refack removed the request for review from bnoordhuisMarch 4, 2019 01:06
@refackrefack 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:masterMar 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: #26280Fixes: #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: #26280Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BridgeARBridgeAR mentioned this pull request Mar 4, 2019
Merged
@BridgeAR

BridgeAR commented Mar 5, 2019

Copy link
Copy Markdown
Member

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

Should this be backported?

@refackrefack removed their assignment Mar 11, 2019
@refack

This comment has been minimized.

Sign in to view
@refack

refack commented Mar 14, 2019

Copy link
Copy Markdown
ContributorAuthor

Backport PR: #26649

@refackrefack self-assigned this Mar 14, 2019
@refackrefack 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#26280Fixes: 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#26280Fixes: 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: #26280Fixes: #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: #26280Fixes: #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: #26280Fixes: #25593
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@BethGriggsBethGriggs mentioned this pull request Apr 9, 2019
Merged
@refackrefack removed their assignment Apr 14, 2019
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleaxaddaleaxaddaleax left review comments
@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues 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