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

build: warn about `--without-ssl` disabling debugger #12768

Closed
wants to merge 2 commits into from

Conversation

@refack
Copy link
Member

commented Apr 30, 2017

  • To fix #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.

Depends on: #12197
Fixes: #12758

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

build, tools, inspector, debugger

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@addaleax question: are changes in our internal tools semver-major?
(for this one it doesn't matter since it's dependent on #12197)

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@addaleax

This comment has been minimized.

Copy link
Member

commented May 1, 2017

are changes in our internal tools semver-major?

No, but ./configure is not an internal tool.

(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

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

No, but ./configure is not an internal tool.

Why?
Does that mean that changing it's stdout output is also semver-major?

(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.)

I know, but there was a conversation somewhere about being explicit about the dont-lands

@refack refack changed the title build: renamed `inspector` switch to `debugger` build: rename `inspector` switch to `debugger` May 1, 2017
@addaleax

This comment has been minimized.

Copy link
Member

commented May 1, 2017

No, but ./configure is not an internal tool.

Why?

Because it ends up being used by hundreds of scripts for building Node.

Does that mean that changing it's stdout output is also semver-major?

Probably not?

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Hmmmm 🤔 gray area, yay (not)...
I'll rework this PR

@refack refack changed the title build: rename `inspector` switch to `debugger` [WIP] build: rename `inspector` switch to `debugger` May 1, 2017
* To fix #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: #12758
@refack refack changed the title [WIP] build: rename `inspector` switch to `debugger` build: require `--without-debugger` explicitly May 1, 2017
@refack refack force-pushed the refack:configure-inspector2debugger branch to 2a158b2 May 1, 2017
@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

Narrowed the score of PR, PTAL. Thank you.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 7, 2017

@sam-github

This comment has been minimized.

Copy link
Member

commented May 8, 2017

So, with this change, every time I build node --without-ssl, I also have to add --without-debugger? If so, I think a extra line in the without-ssl docs describing the impact of not having SSL would be more useful (no crypto, no Inspector, etc.).

@refack

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

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 ./configure run).

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

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

@sam-github like you said, I think we should look into a non TLS proxy for inspector in this scenario...

@refack

This comment has been minimized.

Copy link
Member Author

commented May 8, 2017

@eugeneo since you're coding around there, how feasible is it to proxy inspector over clear TCP if we don't have openSSL?

@refack

This comment has been minimized.

Copy link
Member Author

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-icu

creating 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=all

creating 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 downloaded
Checking file integrity with MD5:
MD5:      29a41f9bb576b06c7eef0487a84a7674  deps\icu4c-59_1-src.zip
 Extracting zipfile: deps\icu4c-59_1-src.zip
* Using ICU in deps/icu
creating icu_config.gypi
@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

So, with this change, every time I build node --without-ssl, I also have to add --without-debugger? If so, I think a extra line in the without-ssl docs describing the impact of not having SSL would be more useful (no crypto, no Inspector, etc.).

How is that congruent with #12723 (comment)? especially when this much less frequently used?
[I'm not arguing, just asking for a rational, since your comment on experimental IMHO was very good]

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 --help line, especially if they already have a build script set up.

@gibfahn

This comment has been minimized.

Copy link
Member

commented May 10, 2017

How is that congruent with #12723 (comment)?

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 ./configure, and if you pass --without-openssl then you get v8_enable_inspector set to 0 rather than 1.

➜  node git:(master) ❯ ./configure --without-ssl
  'variables': { 'node_shared_openssl': 'false',
                 'node_use_openssl': 'false',
                 'v8_enable_inspector': 0, }}
➜  node git:(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.

@gibfahn gibfahn referenced this pull request May 11, 2017
2 of 2 tasks complete
@refack

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

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.

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 --help they didn't change anything, and the build works?

Can we converge on a loud warning message?

@refack refack changed the title build: require `--without-debugger` explicitly build: warn about `--without-ssl` disabling debugger May 11, 2017
@refack

This comment has been minimized.

Copy link
Member Author

commented May 11, 2017

just FYI my console after python configure
image
scrolling up
image

@refack

This comment has been minimized.

Copy link
Member Author

commented May 19, 2017

dies of natural causes

@refack refack closed this May 19, 2017
@refack refack deleted the refack:configure-inspector2debugger branch May 19, 2017
@joshgav joshgav removed the diag-agenda label May 19, 2017
gibfahn added a commit to gibfahn/node that referenced this pull request May 22, 2017
- 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 <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@refack refack restored the refack:configure-inspector2debugger branch May 25, 2017
refack added a commit to refack/node that referenced this pull request May 26, 2017
- 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 <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
jasnell added a commit that referenced this pull request May 28, 2017
- 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 <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@refack refack deleted the refack:configure-inspector2debugger branch Jun 10, 2017
MylesBorins added a commit that referenced this pull request Jul 17, 2017
- 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 <riclau@uk.ibm.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.