fix: node.js debugger adds stderr (but exit code is 0) -> shouldn't throw

[FuPeiJiang]

Debugger attached.
Waiting for the debugger to disconnect...

Checklist

• npm install && npm test passes
• tests are included
• documentation is changed or added
• commit message follows commit guidelines

Description of change

[cclauss]

Suggested change

	if p.wait() != 0:
	sys.stderr.write(p_stderr)
	# Simulate check_call behavior, since check_call only exists
	# in python 2.5 and later.
	if p_stderr:
	sys.stderr.write(p_stderr)
	if p.wait() != 0:

[cclauss]

Is there a simple way that this code can detect the presence of the debugger?

[cclauss]

If we are going to make changes here, the recommended path is to upgrade to subprocess.run() to remove a lot of clutter.

[FuPeiJiang]

keep comments ? (I think it's for the raise below) (keep/delete) ?

                        # Simulate check_call behavior, since check_call only exists
                        # in python 2.5 and later.
                        raise GypError(
                            "Call to '%s' returned exit status %d while in %s."
                            % (contents, p.returncode, build_file)
                        )

this is better behavior, no loss of stderr output (possible useful debug log)

[cclauss]

We do not need the comment about Py25... subprocess.check_cal() is fully available to us on all supported versions of Python and the even simpler subprocess.run() is too.

Will all users have the debugger present? If not, what is the effect of the proposed changes on those users?

[FuPeiJiang]

what is the effect of the proposed changes on those users?

<!(node -p \"require('node-addon-api').gyp\")

oof, the scope is Any command..

---

Is there a simple way that this code can detect the presence of the debugger?

there may be another way: silence the:

Debugger attached.
Waiting for the debugger to disconnect...

https://github.com/nodejs/node/blob/fddc701d3c0eb4520f2af570876cc987ae6b4ba2/src/inspector_agent.cc#L802-L805
It seems that it only prints if no parent
python spawning seems like a parent ? I'll have to check more

[FuPeiJiang]

Stderr doesn’t mean error,
Status code > 0 means error

Yes, programs can output to standard error and still return a zero exit status. You will find many POSIX utilities specifications which state that "The standard error shall be used only for diagnostic messages."

https://unix.stackexchange.com/a/594670

this is the correct handling

If any program wants to indicate error, just set status code

[cclauss]

Suggested change

	# Simulate check_call behavior, since check_call only exists
	# in python 2.5 and later.

Remove.

[FuPeiJiang]

the tests aren't even re-ran after force-push, this broke 2 tests

[FuPeiJiang]

it's checking if the last line of stderr === 'gyp info ok'
but now it's === 'Waiting for the debugger to disconnect...'

[FuPeiJiang]

nevermind, no breaking changes,
these 2 tests were always meant to fail in JavaScript Debug Terminal
and they were always meant to succeed in Not JavaScript Debug Terminal
(has nothing to do with input.py)

[FuPeiJiang]

I feel that checking the last line of something isn't a foolproof way of checking for success, maybe exit code ? I'll have to check the intention

[cclauss]

Add text = True to the run() call and then...

Suggested change

	replacement = result.stdout.decode("utf-8").rstrip()
	replacement = result.stdout.rstrip()

[cclauss]

Suggested change

	check=False
	check=False,
	text=True

[FuPeiJiang]

fails python 3.6 tests
revert change text=True

Changed in version 3.7: Added the text parameter, as a more understandable alias of universal_newlines. Added the capture_output parameter.

https://docs.python.org/3/library/subprocess.html#:~:text=Changed%20in%20version%203.7%3A%20Added%20the%20text%20parameter%2C%20as%20a%20more%20understandable%20alias%20of%20universal_newlines.%20Added%20the%20capture_output%20parameter.

[cclauss]

LGTM (Once all tests are passing ;-) )

[cclauss]

Yes. Py36 is end-of-life https://devguide.python.org/versions so we should drop support for it but let's move this forward as is.

[rvagg]

this shouldn't have been merged here but over in gyp-next: https://github.com/nodejs/gyp-next/blob/main/pylib/gyp/input.py

I believe it's going to get overwritten with a upgrade. @FuPeiJiang would you mind doing an equivalent PR over there please?