tools: allow running test.py without configuring #16621
Conversation
nodejs-github-bot
added
test
gibfahn
force-pushed
the
test-no-config
branch
2 times, most recently
from
6dd1b67 to
9cc9544
Compare
tools/test.py
Outdated
| config_gypi = ast.literal_eval(s) | ||
| return config_gypi['variables']['v8_enable_inspector'] | ||
|
|
||
| try: |
There was a problem hiding this comment.
why not replace with exec(vm + '-p process.config.variables.v8_enable_inspector')
There was a problem hiding this comment.
Not for this PR but I wonder if we should add the inspector to process.features.
Trott
added
the
python
|
@refack good idea, implemented. cc/ @nodejs/testing @nodejs/python |
Someone should finish with nodejs/build#879 so we could CI this 🙄 |
| has_inspector = Execute([vm, | ||
| "-p", "process.config.variables.v8_enable_inspector"], context) | ||
| if has_inspector.stdout.rstrip() == "0": | ||
| context.v8_enable_inspector = False |
There was a problem hiding this comment.
Does this set context.v8_enable_inspector if process.config.variables.v8_enable_inspector is falsy?
There was a problem hiding this comment.
Only if it's specifically 0, not just generally falsy.
There was a problem hiding this comment.
I guess this is okay as long as the only valid values for process.config.variables.v8_enable_inspector are 0 and non-zero.
There was a problem hiding this comment.
AFAICT possible values are 0, 1 with undefined in v6 and v4.
There was a problem hiding this comment.
For v6 it's process.config.variables.v8_inspector
There was a problem hiding this comment.
Okay, I'd be inclined to leave this as is here, and just not backport to 4.x and 6.x. Seem reasonable?
There was a problem hiding this comment.
The V8 inspector support isn't in v4.x so no need to backport there.
|
(ignore the centos6 fails) |
If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: nodejs#16621 Refs: nodejs#16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
gibfahn
force-pushed
the
test-no-config
branch
2 times, most recently
from
aba2e63 to
ad1967d
Compare
If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: #16621 Refs: #16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
|
This doesn't land cleanly on v6.x-staging, would you be willing to manually backport |
If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: #16621 Refs: #16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
If config.gypi isn't defined, assume Node was build the default way, i.e. with the inspector. PR-URL: #16621 Refs: #16436 (comment) Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>
|
This doesn't need to be backported as the relevant commit from #11631 wasn't backported. If the rest of that PR gets backported we should include this. |
If config.gypi isn't defined, assume Node was build the default way,
i.e. with the inspector.
Refs: #16436 (comment)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
test