Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.2k
build: warn about --without-ssl disabling debugger#12768
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
refack commented Apr 30, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
refack commented May 1, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
refack commented May 1, 2017
/cc @nodejs/build @nodejs/diagnostics |
addaleax commented May 1, 2017
No, but (Also, just so you know, it doesn’t really make a difference to add dont-land labels for semver-major changes; it’s kind of implicit in the label that it shouldn’t land on any release branches.) |
refack commented May 1, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Why?
I know, but there was a conversation somewhere about being explicit about the |
inspector switch to debuggerinspector switch to debuggeraddaleax commented May 1, 2017
Because it ends up being used by hundreds of scripts for building Node.
Probably not? |
refack commented May 1, 2017
Hmmmm 🤔 gray area, yay (not)... |
inspector switch to debuggerinspector switch to debugger* To fixnodejs#12758, added an assertion that if `--without-ssl` or `--without-intl` are set, --without-debugger must also be explicitly set. * Added `--without-debugger` is an alias to `--without-inspector`, since we removed the old TCP `debug` protocol the V8 `inspect` protocol is the only debugger available. Hence disabling `inspector` means disabling any kind on JS debugger. Fixes: nodejs#12758
inspector switch to debugger--without-debugger explicitly69fdc51 to 2a158b2Comparerefack commented May 1, 2017
Narrowed the score of PR, PTAL. Thank you. |
refack commented May 7, 2017
ping @nodejs/build @nodejs/diagnostics @danbev@sam-github |
sam-github commented May 8, 2017
So, with this change, every time I build node |
refack commented May 8, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Do you mean https://github.com/nodejs/node/blob/master/configure#L433? IMHO that's very subtle (even if with couple it with emitting a warning message on IMHO a debugger interface is a very important feature, even in a production environment. For me it's my eyes and ears into any kind of problems, but that's a personal opinion. |
refack commented May 8, 2017
@sam-github like you said, I think we should look into a non TLS proxy for |
refack commented May 8, 2017
@eugeneo since you're coding around there, how feasible is it to proxy |
refack commented May 9, 2017
@sam-github Apperently I didn't invent it 😉 c:\code\node-cur$ python configure --dest-cpu=x64 --tag= --ninja --with-intl=full-icucreating icu_config.gypi* ECMA-402 (Intl) support didn't find ICU in deps/icu..Warning: Not downloading package "icu". You could pass "--download=all" (Windows: "download-all") to try auto-downloading it.Cannot build Intl without ICU in deps/icu.(Fix, or disable with "--with-intl=none" )c:\code\node-cur$ python configure --dest-cpu=x64 --tag= --ninja --with-intl=full-icu --download=allcreating icu_config.gypi* ECMA-402 (Intl) support didn't find ICU in deps/icu.. <https://ssl.icu-project.org/files/icu4c/59.1/icu4c-59_1-src.zip> Fetch: : 36.4MB total, 36.4MB downloadedChecking file integrity with MD5:MD5: 29a41f9bb576b06c7eef0487a84a7674 deps\icu4c-59_1-src.zip Extracting zipfile: deps\icu4c-59_1-src.zip* Using ICU in deps/icucreating icu_config.gypi |
refack commented May 10, 2017
How is that congruent with #12723 (comment)? especially when this much less frequently used? If the setup scripts were silent then I agree a warring would have good effect, but they are noisy and I'm afraid it will not be noticed, also no one will read the |
gibfahn commented May 10, 2017
That's about end users of node, not people building it. We generally tend to assume that people building node will read the docs and check warnings. The list of enabled/disabled components is the output of ➜ nodegit:(master)❯./configure--without-ssl'variables': {'node_shared_openssl': 'false','node_use_openssl': 'false','v8_enable_inspector': 0,}}➜ nodegit:(master)❯./configure'variables': {'node_shared_openssl': 'false','node_use_openssl': 'true','v8_enable_inspector': 1,}}I'm -1 on this because we didn't treat the inspector as just a breaking change to the debugger, we've treated it as an entirely new thing. |
refack commented May 11, 2017 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
IMHO it's a synthetic distinction. I'm imagine a 3d party that has a build script, now with node8, their product does not have a debugger interface, while they didn't change their build setup... Why should they read the Can we converge on a loud warning message? |
--without-debugger explicitly--without-ssl disabling debuggerrefack commented May 11, 2017
refack commented May 19, 2017
dies of natural causes |
- The V8 inspector is no longer experimental. - Note that building without SSL disables other features. PR-URL: nodejs#12978 Refs: nodejs#12768 (comment) Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
- The V8 inspector is no longer experimental. - Note that building without SSL disables other features. PR-URL: nodejs#12978 Refs: nodejs#12768 (comment) Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
- The V8 inspector is no longer experimental. - Note that building without SSL disables other features. PR-URL: #12978 Refs: #12768 (comment) Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
- The V8 inspector is no longer experimental. - Note that building without SSL disables other features. PR-URL: #12978 Refs: #12768 (comment) Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>


To fixbuild: --without-ssl and --without-intl imply new limitation (no debugger) #12758, added an assertion that if
--without-sslor--without-intlare set, --without-debugger mustalso be explicitly set.
Added
--without-debuggeris an alias to--without-inspector, sincewe removed the old TCP
debugprotocol the V8inspectprotocol is theonly debugger available. Hence disabling
inspectormeans disablingany kind on JS debugger.
Depends on: #12197
Fixes: #12758
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
build, tools, inspector, debugger