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

lib: make sure console is writable #20185

Closed
wants to merge 0 commits into from
Closed

Conversation

kfarnung
Copy link
Contributor

@kfarnung kfarnung commented Apr 21, 2018

The code currently assumes that console is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

Refs: #17708

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

@kfarnung kfarnung self-assigned this Apr 21, 2018
@kfarnung
Copy link
Contributor Author

@devsnek
Copy link
Member

devsnek commented Apr 21, 2018

@kfarnung this is because of a change in node-chakra right?

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Would be cool to have a test for this, if it's possible?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM. +1 to adding a test.

@kfarnung
Copy link
Contributor Author

@devsnek That's basically correct. At some point V8 moved their console object initialization out of the inspector code so they always initialize the console property as writable. In the case of node-chakracore we still only initialize the console object when inspector is included in the build. Since the intention is to have the property be always writable it seemed reasonable to just have this code ensure that the property is always writable.

@addaleax @cjihrig There are already existing tests for the behavior, but they fail in node-chakracore if we build without inspector (which is currently true for our WoA builds). See #17708 which added those tests. I'll add ref to that PR to this commit as well.

@kfarnung
Copy link
Contributor Author

Rebased and kicked off a new CI: https://ci.nodejs.org/job/node-test-pull-request/14421/

@BridgeAR BridgeAR added console Issues and PRs related to the console subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 21, 2018
@kfarnung
Copy link
Contributor Author

The CI failures seem unrelated (there should be no functional change here):

https://ci.nodejs.org/job/node-test-commit-plinux/17100/nodes=ppcle-ubuntu1404/consoleText

match failed
line=6
expect=^oooooooooooooooooooooooooooooooooooooooooooooooo$
actual=oo����oooooooooooooooooooooooooooooooooooooooooo
not ok 2217 pseudo-tty/no_interleaved_stdio
  ---
  duration_ms: 0.115
  severity: fail
  stack: |-
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oo����oooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    oooooooooooooooooooooooooooooooooooooooooooooooo
    ooooooooooooooooooooooooO__This is some stderr__
  ...

https://ci.nodejs.org/job/node-test-commit-osx/18000/nodes=osx1010/consoleText

not ok 652 parallel/test-child-process-fork-net2
  ---
  duration_ms: 120.429
  severity: fail
  exitcode: -15
  stack: |-
    timeout
  ...

@kfarnung
Copy link
Contributor Author

@kfarnung
Copy link
Contributor Author

Looks like the macOS leg has some leftover processes:

# Clean up any leftover processes, error if found.
ps awwx | grep Release/node | grep -v grep | cat
67423   ??  R     32:31.66 /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/out/Release/node /Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-listen-fd-detached-inherit.js parent
make[1]: *** [test-ci] Error 1
make: *** [run-ci] Error 2
Build step 'Execute shell' marked build as failure

Seems to be another instance of #20194.

kfarnung added a commit that referenced this pull request Apr 24, 2018
The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

Refs: #17708

PR-URL: #20185
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@kfarnung kfarnung closed this Apr 24, 2018
@kfarnung
Copy link
Contributor Author

Landed in 982adb5

@kfarnung kfarnung deleted the console branch April 24, 2018 00:29
MylesBorins pushed a commit that referenced this pull request May 4, 2018
The code currently assumes that `console` is already writable, but
that's only if it was previously defined as writable. If it hasn't
already been defined then the default value is false.

Refs: #17708

PR-URL: #20185
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants