FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Revert "vm: add importModuleDynamically option to compileFunction" by mcollina · Pull Request #33364 · nodejs/node · GitHub

/ node Public

Revert "vm: add importModuleDynamically option to compileFunction" - #33364

Closed
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:revert-#32985
ConversationCommits (1)ChecksFiles changed
Closed

Revert "vm: add importModuleDynamically option to compileFunction"#33364
mcollina wants to merge 1 commit into
nodejs:masterfrom
mcollina:revert-#32985

Conversation

mcollina commented May 12, 2020

Copy link
Copy Markdown
Member

This reverts commit 74c393d.

Fixes: #33166
See: #32985
See: #31860

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

mcollina requested review from BethGriggs, devsnek and targos May 12, 2020 09:22
nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. vm Issues and PRs related to the vm subsystem. labels May 12, 2020

mcollina commented May 12, 2020

Copy link
Copy Markdown
Member Author

@SimenB this should likely fix Babel and Jest.

mcollina mentioned this pull request May 12, 2020
Closed

SimenB commented May 12, 2020

Copy link
Copy Markdown
Member

I hope it can re-land later as I wanna use the code this reverts. 😀 But it does seem prudent to revert for now.
I wonder if the problem points to some other deep-lying issue that this change just exposed, though. If building using a different compiler changes things it seems likely this JS change is just a symptom and not the cause.

nodejs-github-bot commented May 12, 2020

Copy link
Copy Markdown
Collaborator

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

targos reviewed May 12, 2020
View reviewed changes
Comment thread doc/api/vm.md Outdated
benjamingr approved these changes May 12, 2020
View reviewed changes

benjamingr left a comment •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Rubber stamp LGTM due to revert + severity.

(Plus a non-rubber-stamp-lgtm on the delta like the md changes)

mcollina commented May 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

I wonder if the problem points to some other deep-lying issue that this change just exposed, though. If building using a different compiler changes things it seems likely this JS change is just a symptom and not the cause.

I literally have no clue on how to debug that. I know there is something in vs2019 that is affecting stdout, but it's really very hard to track. cc @ronag #32487

targos commented May 12, 2020

Copy link
Copy Markdown
Member

@mcollina is #33166 also reproducible on nightlies from master? If not, I think we should actually only revert on v14.x.

mcollina commented May 12, 2020

Copy link
Copy Markdown
Member Author

It fails also on the nightlies

devsnek commented May 12, 2020

Copy link
Copy Markdown
Member

I still don't get why we consider this commit the issue and not vs2019

mcollina commented May 12, 2020

Copy link
Copy Markdown
Member Author

I still don't get why we consider this commit the issue and not vs2019

@devsnek because this commit triggers the problem. Node v14 is largely broken on one of the supported Operating Systems, and it is important that we ship a fix sooner rather than later. However I have got no clue on how to approach the vs2019 issue and it might take an unbounded amount of time to fix it. We currently have no strategy around this problem.

I'm currently pinging some connections in Microsoft to see if they can help, but I fear it might take a long time to get to a conclusion as this problem is very ephemeral.

devsnek commented May 12, 2020

Copy link
Copy Markdown
Member

@mcollina I mean why are we saying "build without this commit" instead of "build without vs2019". This issue could easily manifest itself in other ways (I think jasnell mentioned stream timing bugs?) so it seems like we should use a better compiler.

mcollina commented May 12, 2020

Copy link
Copy Markdown
Member Author

I do not know how long would it take for the @nodejs/build to change the compiler we are using to build our Node.js releases. I suspect it's a significant amount of effort. I do not think we can wait fixing this bug on v14.

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 12, 2020

addaleax commented May 12, 2020

Copy link
Copy Markdown
Member

I’m a bit worried here too … I get that this is a severe problem, but without understanding why this commit causes it – it almost certainly only exposes a pre-existing bug – there’s a decent chance that we’ll do some other thing that re-exposes it in the future without knowing.

jasnell commented May 12, 2020

Copy link
Copy Markdown
Member

Yep. I consider this revert to be temporary to fix the acute breakage but the revert itself is not the solution.

mhdawson approved these changes May 12, 2020
View reviewed changes

mhdawson 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

cjihrig mentioned this pull request May 14, 2020
Closed
2 tasks

nodejs-github-bot commented May 15, 2020

Copy link
Copy Markdown
Collaborator

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

mcollina commented May 15, 2020

Copy link
Copy Markdown
Member Author

Landed in 2d5d773

mcollina closed this May 15, 2020
mcollina added a commit that referenced this pull request May 15, 2020
Revert "vm: add importModuleDynamically option to compileFunction"
2d5d773 
This reverts commit 74c393d.

Fixes: #33166

PR-URL: #33364
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere pushed a commit that referenced this pull request May 16, 2020
Revert "vm: add importModuleDynamically option to compileFunction"
c2cf978 
This reverts commit 74c393d.

Fixes: #33166

PR-URL: #33364
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
codebytere mentioned this pull request May 18, 2020
Merged
devsnek mentioned this pull request Sep 30, 2020
Merged
4 tasks
devsnek added a commit that referenced this pull request Feb 5, 2021
vm: add importModuleDynamically option to compileFunction
bf2f2b7 
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
vm: add importModuleDynamically option to compileFunction
66a14d3 
This reverts commit 2d5d773.

See: #32985
See: #33364
See: #33166
Fixes: #31860

PR-URL: #35431
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Ujjwal Sharma <ryzokuken@disroot.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
benjamingr benjamingr approved these changes
cjihrig cjihrig approved these changes
BethGriggs BethGriggs approved these changes
BridgeAR BridgeAR approved these changes
devsnek Awaiting requested review from devsnek
+2 more reviewers
targos targos left review comments
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. tsc-agenda Issues and PRs to discuss during the meetings of the TSC. vm Issues and PRs related to the vm subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Regression in 14.1.0 - Windows] stdout is sometimes empty
Back | FazBrowse Home | New Git URL