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

[v11.x] process: provide dummy stdio for non-console Windows apps #25356

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jan 5, 2019

Clean cherry-pick of #20640, it’s just macOS CI that this may or may not end up being blocked on.


The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which uv_guess_handle fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

The only known condition where we could not provide appropriate
stdio streams so far were non-console Windows applications.
Since this issue has come up a few times in our issue tracker now,
switch to providing dummy streams for these cases instead.

If there are other valid cases in which `uv_guess_handle` fails,
and where there is a more sensible way to provide stdio,
we’ll probably still find out because the streams don’t work
properly either way.

Refs: nodejs/help#1251

PR-URL: nodejs#20640
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. process Issues and PRs related to the process subsystem. v11.x labels Jan 5, 2019
@addaleax
Copy link
Member Author

addaleax commented Jan 5, 2019

@addaleax addaleax removed the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Jan 5, 2019
@addaleax
Copy link
Member Author

addaleax commented Jan 5, 2019

/cc @nodejs/platform-macos for investigating the failure

@bzoz
Copy link
Contributor

bzoz commented Jan 7, 2019

For Windows, the compilation issue is strange:

23:05:08   Traceback (most recent call last):
23:05:08     File "..\src\inspector\build\xxd.py", line 28, in <module>
23:05:08       sys.exit(main())
23:05:08     File "..\src\inspector\build\xxd.py", line 24, in main
23:05:08       with open(output_filename, 'w') as output_file:
23:05:08   IOError: [Errno 13] Permission denied: 'c:\\workspace\\node-compile-windows\\Release\\obj\\global_intermediate\\src\\inspector\\injected-script-source.h'
23:05:08 C:\Program Files (x86)\Microsoft Visual Studio\2017\Community\Common7\IDE\VC\VCTargets\Microsoft.CppCommon.targets(171,5): error MSB6006: "cmd.exe" exited with code 1. [c:\workspace\node-compile-windows\deps\v8\gypfiles\v8_base_0.vcxproj]

Maybe just a fluke needing a retry?

/cc @nodejs/build @nodejs/platform-windows

@addaleax
Copy link
Member Author

addaleax commented Jan 7, 2019

@bzoz Yeah, I wouldn’t worry about that for now… marking this as WIP while it’s failing on macOS

@addaleax addaleax added wip Issues and PRs that are still a work in progress. macos Issues and PRs related to the macOS platform / OSX. labels Jan 7, 2019
@Trott
Copy link
Member

Trott commented Jan 9, 2019

@addaleax Can reproduce failure locally on macOS. Going to bisect master to see if I can identify the commit that fixed it there.

@Trott
Copy link
Member

Trott commented Jan 9, 2019

It appears that 3337836 is the commit that fixed things so that this change no longer caused test failures on macOS. So maybe if that backport is done and landed first, this one will pass on macOS?

@BridgeAR
Copy link
Member

BridgeAR commented Jan 9, 2019

@Trott thanks for investigating! I am going to land the mentioned backport later, so let's see if we are able to land this afterwards.

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LG if the CI is green.

@BridgeAR
Copy link
Member

Landed in 8d166c9 🎉 (OS-X is finally green due to the console commit).

@BridgeAR BridgeAR closed this Jan 10, 2019
@BridgeAR BridgeAR removed the wip Issues and PRs that are still a work in progress. label Jan 10, 2019
@addaleax addaleax deleted the backport-20640 branch January 12, 2019 18:31
@addaleax
Copy link
Member Author

@Trott @BridgeAR I’m not sure – this does not sound like the actual underlying issue that caused the crashs has been taken care of. It’s nice that CI is green though, and I don’t think many people would be running into problems (esp. on OS X, as this primarily targets Windows).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macos Issues and PRs related to the macOS platform / OSX. process Issues and PRs related to the process subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants