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

Uniform way to trigger debugger on first line #12630

Closed
refack opened this issue Apr 24, 2017 · 49 comments

Comments

Projects
None yet
@refack
Copy link
Member

commented Apr 24, 2017

Ref: #12364

  1. Have --inspect --debug-brk as a uniform way to trigger debug on first line
    1. Restore the --inspect --debug-brk combo to v7+v8 (#12580)
    2. Does it stay undocumented? Or maybe it should become the primary option and remove --inspect-brk (re: #11441)
      (IMHO if we keep it --debug-brk, --inspect-brk will probably never be used)
    3. Will the combo be valid in node8?
    4. What do we do with --debug-port / inspect-port?
    5. In light of these, consider what to mark as deprecated.
    6. Since v6 and v7 requires the combo --inspect[=port] --debug-brk[=port] is specifying port on both args a valid invocation, and in that case which port wins?
  2. Have --inspect-brk as a uniform way to trigger debug on first line
    1. Keep the --inspect --debug-brk combo in v7 alone (deprecation notice yes/no) (i.e. don't land #12197 in v7)
    2. port the --inspect-brk alias to v6 (#12615)
      [new comment] this will make --inspect-break a feature of recent versions of 6.x, but it can't change the past: versions of 6.x will always exist without this feature, and so will not be debuggable by third-party tooling [without special treatment]
    3. for v4 it's irrelevant since it's a different protocol and other means of detection and handling is necessary
  3. Help the users adapt to our plan:
    1. Help fix VSCode properly, make it compatible with our current and future plans (/cc @roblourens Ref: microsoft/vscode-node-debug2#100)
    2. Make sure JetBrains handle node8 nightlies (/cc @ulitink @segrey youtrack#WEB-26568)

User feedback

I'm trying to get more feedback from @roblourens and JetBrains, so you could make the best decision.

  1. Quote from youtrack#WEB-26568
    image

  2. Comment from @roblourens #12364 (comment)

P.S. at present WebStorm and IDEA based IDEs can't trigger debug in node8 nightlies (nor can VSCode)

@refack

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

After hearing from the IDE vendors, IMHO we should forget about --inspect-brk, it won't get adopted, and just keep --debug-brk permanently.
Protocol detection & resolution is done in orthogonal ways anyway.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I'm OK with keeping --debug-brk permanently as the flag meaning "break-on-first-line" (too bad it wasn't called that originally). It seems analogous (to me) with how node debug starts the inspector in most recent. In which case, we don't have to backport anything, we just start the deprecation process for --inspect-brk (so long, barely had time to know you).

I'm also OK with backporting --inspect-brk.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

It seems to me that if you ignore the --inspect --debug-brk combo, then the situation becomes much more simple. Going forward we want a flag that means "start the debugger and break on the first line", and we have two options. The pros and cons are:

Choice Pros Cons
--inspect-brk --inspect+--inspect-brk makes sense it's another option to remember, --debug-brk already exists
--debug-brk Old flag, new protocol, just keeps working. Also more intuitive if you don't know or care what the inspector is. May be confusing that --debug-brk on v6 !== --inspect --debug-brk on v6 and !== --debug-brk on v7

Thinking about this further, I'd be +1 for abandoning --inspect-brk and sticking with --debug-brk. By Node 10 or 11, no-one will (hopefully) remember or need to care about the difference between the two protocols, we'll just have the one. And at that point having --debug-brk mean "start a debugger and break" seems like the natural choice.

If we're going to do that then we should not backport --inspect-brk, and we should re-add --debug-brk to master.

EDIT: I'd also say that the number of times I've used the inspector without the brk option is very small, I'd say this is really the default option for a user.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I'm OK with keeping --debug-brk permanently as the flag meaning "break-on-first-line" (too bad it wasn't called that originally).

@sam-github the thing is that you're not supposed to do --inspect --debug-brk, you're just supposed to do --debug-brk, which debugs and breaks. So it actually seems pretty well named to me. The --inspect --debug-brk thing is a temporary aberration caused by having two debug protocols in one version of Node.

@refack

This comment has been minimized.

Copy link
Member Author

commented Apr 24, 2017

@sam-github the thing is that you're not supposed to do --inspect --debug-brk, you're just supposed to do --debug-brk, which debugs and breaks. So it actually seems pretty well named to me. The --inspect --debug-brk thing is a temporary aberration caused by having two debug protocols in one version of Node.

If you look at the code it's actually syntactic sugar for three operations:

  1. Choose protocol
  2. Set port
  3. Break on first line

I think ideally it would have only been used by users, while vendors used the three explicit args
--inspect --brake-on-first --debugger-port=5599

BTW: should it be "break" of "brake"?

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 24, 2017

I think it's break as in breakpoint.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@Trott @eugeneo thoughts? doesn't matter to much too me which we do, but we need to do one or the other, allow --debug-brk in all versions, or backport --inspect-break.

@Trott

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@Trott @eugeneo thoughts? doesn't matter to much too me which we do, but we need to do one or the other, allow --debug-brk in all versions, or backport --inspect-break.

I'm happy to defer to the judgment of @nodejs/diagnostics and @nodejs/LTS on this.

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@refack

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

The IDE vendors are now making it clear a breaking change in args will not be backported:

JetBrains: #12364 (comment)
image

VScode: #12364 (comment)
image

(the 👍s is mine)

Seems we'll have to keep --inspect --debug-brk for the time being. So IMHO --inspect-brk and inspect-port are redundant...

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

My personal, pedantic vote is --break-on-first-line.

Alternatively, --{debug|inspect}-pause probably make more sense.

... I suppose the ship has already sailed on that though, so maybe we should just keep --debug-brk because it is essentially just adding a debugger; which shares a similar name-ism.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

I'm OK with @refack and @Fishrock123 's suggestion of keeping --debug-brk and --debug-port indefinitely as the documented CLI options.

What about the briefly existing --inspect-brk and --inspect-port, leave them indefinitely, but undocumented? Or should we start the deprecation process (doc deprecate for 8.x, runtime deprecate for 9.x, remove in 10.x)?

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

I think --debug-port should be done away with in favor of --inspect-port, fwiw.

@roblourens

This comment has been minimized.

Copy link

commented Apr 25, 2017

I wish we had added --inspect-brk from the beginning - I agree at least that it looks nicer and is consistent with --debug-brk. But I also wonder, in the future when the old protocol is long forgotten, will --inspect be a good name? If someone is looking for an option to "debug", will they discover it? I wonder where it will make sense to add --debug as an alias at some point.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

@Fishrock123 so, you want node debug, --inspect, --inspect-port, and --debug-brk?

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

--inspect-brk is the only way to start the mostly-desired debug behavior with one single flag. I really don't think we should remove it. I'd be fine with keeping --debug-brk/--debug-port around as a deprecated features as long as it's only valid in combination with --inspect. I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

No I would rather it all just be inspect but I guess we might have to make a compromise somewhere.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

I would prefer --inspect, --inspect-port and --inspect-brk. We can add --inspect-port and --inspect-brk as aliases of --debug-brk and --debug-port in LTS streams, and --debug-brk and --debug-port as undocumented, deprecated aliases of their --inspect-* counterparts going forward.

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234.

@jkrems You wouldn't do that, in Node 8 node --debug-brk=1234 would be identical to node --inspect-brk=1235 (which is why we only want to keep one). So the question is --debug-brk or --inspect-brk.

@sam-github

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

We can have it all be inspect, we just have to backport --inspect-brk, and support --debug-brk going forward for a while. Maybe the CTC needs to hash this out f2f.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

@gibfahn But then a tool that passes only --debug-brk={myPort} would see widely different behavior between {any other version of node} and node 8. And there wouldn't be any good indication for why it broke. Having the same set of command line flags enable entirely different protocols is a bit too evil for my taste.

@refack

This comment has been minimized.

Copy link
Member Author

commented Apr 25, 2017

I'm not sure why anyone in the "glorious future" would prefer --inspect --debug-brk --debug-port=1234 over --inspect-brk=1234

@jkrems I think we have two use-cases:

  1. Automated tools: who might prefer explicit 3 part arg:
    1.protocol - --inspect/debug
    2.should-stop-on-first yes/no - --debug-brk (if we could give it a better name we should)
    3.which port --debug-port=1234
    IMHO this combination can be formed in a simpler if-else process.
  2. CLI user - who probably prefer the shortcut --inspect-brk=1234, can make complex decisions and adapt to new syntax.
@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@billti @roblourens @prigara we would like to hear your latest opinions on two things:

  1. Is there consensus that --inspect --debug-brk is necessary for compatibility with existing tools?
  2. What is your preference for future syntax?
    1. adopt single argument --inspect-brk[=[host:]port], and backport it?
    2. adopt the explicit three argument combo (protocol, port, is-to-break)
      --inspect --debug-port=HOST:XXX --debug-brk?
    3. keep the 2 argument combo --inspect --debug-brk for the foreseeable future?
@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

I thought the general agreement was:

  1. We need to keep --inspect --debug-brk working for the time being.
  2. For better usability & less confusion --inspect-brk is still a good thing and - imo - should definitely be ported to node 6.

This means future tools (after node 6 phased out) won't have to deal with any "why are halve of the flags called --inspect and the other half --debug", users get CLI flags as nice & succinct as the original --debug/--debug-brk, and older IDEs will work against node 8.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

@jkrems I also think there is wide agreement on 1
As for 2 I have an assumption the vendors will not adopt --inspect-brk, that's why I asked them.
As for individual CLI users, I have no strong opinion, just the preference that we have a single set of well-documented arguments (so we might need to yield with the vendors 🤷)

@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

@refack The thing is: node --inspect-brk is a lot less annoying to type than node --inspect --debug-brk. I don't quite get why we would "willfully" regress on the CLI usability, especially given that on current master node --inspect-brk already works. I get keeping the --debug-brk alias around. But not the other part.

P.S.: Even if today's vendors won't use --inspect-brk, in 1 or 2 years different people will build different tools targeting different (read: newer) versions of node. And I'd be surprised if they pick --inspect --debug-brk over --inspect-brk if they have the choice.

@roblourens

This comment has been minimized.

Copy link

commented May 1, 2017

I agree with @jkrems. As long as --inspect --debug-brk works for the foreseeable future, I'm happy. --inspect-brk seems good to keep as well.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

I kind of agree.
As I see it, the message should be is: for v6 & v7 we had to fast transition our debug protocol. Sorry for confusing our users with new names. node8 has only one debugger and only one (official) way to trigger it (undocumented backward compatibility aliases are a "secret" necessary evil).
What I hope for node8 is to make --inspect-brk == --debug-brk since there is no other debugger.

My personal preference is to make the debug* variants the official variants (IMHO the inspect moniker was as internal name that got leaked to userland).

P.S. voice your opinion on our internal issue #12768

@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

As pointed out elsewhere in the thread, going back to --debug (or --debug-brk w/o --inspect) anytime soon is pretty evil. The same command line option combination shouldn't trigger 2 completely different protocols across different versions of node. Especially when we have node 6 and 7 where the two protocols coexist.

--inspect* is for the "V8 inspector protocol". --debug* is for the "V8 debugging protocol". There are good reasons to use different flags for different protocols. --inspect --debug-brk is an unfortunate weirdness for historical reasons but we should try to reduce confusion for future releases.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 1, 2017

I want to reduce bikeshedding...
IMHO The main function of --inspect* and --debug* is to start a debugger.
inspect and debug are different protocols, and that's secondary. Similar to --inspect:9999 and --nspect:1234 listen to different ports.
@jkrems if you still strongly disagree, I yield (pending words from the vendors).

I think we can all agree that the docs and node --help should only display one set (or something with [deprecated] as necessary).

@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 1, 2017

I think we can all agree that the docs and node --help should only display one set (or something with [deprecated] as necessary).

Yes, those official parameters are --inspect* for the inspect protocol and, for older versions of node, --debug* for V8's old protocol. For versions that support both, node --help would display both. For versions that only support one, it would display one. If somebody explicitly passes --xyz to start one of the protocols, they want to start one specific protocol that their client supports. Otherwise they would click a button in their IDE which in turn would pass specific flags to expose the protocol it expects.

For me the fact that we'll have to keep --inspect --debug-brk around to support "early" adopters of the inspector protocol is the internal detail that we shouldn't leak into our docs & --help output.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

@Fishrock123 during the transition when both protocols were avalible, all vendors had a version update that was compatible with both. There was no back-porting :(

As mentioned above by all vendors, current versions will not be able to debug node8, and they firmly state that they will not backport. So users will have to upgrade/buy a new IDE

@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

All current versions are compatible with both protocols

@Trott Trott added the ctc-agenda label May 10, 2017

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Cross-posting from: #12580 (comment)

-1, I don't really see the point in keeping it when --debug-brk was going to be removed in a major.

I would be for having it print out a notice to use --inspect-brk though.


the vendors can't/won't backport --inspect-brk so all current IDEs will not be albe to debug node8

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

I'll try to sum:

  1. current versions of IDEs work with both protocols
  2. vendors have been exclusively using --inspect --debug-brk to trigger inspector
  3. if we don't restore alias current IDEs will not work with node8
  4. Give vendors a year to shift
@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

the vendors can't/won't backport --inspect-brk so all current IDEs will not be albe to debug node8

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

#12630 (comment)

They all had a release cycle while both protocols were available. Unfortunately they used --inspect --debug-brk to trigger inspector. That's hardcoded logic in all current versions.

@jkrems

This comment has been minimized.

Copy link
Contributor

commented May 10, 2017

If they can "back-port" --inspect and detect that it is a version that needs the inspector, what is stopping them for doing the same for the similar *-brk flag?

To elaborate on what @refack already said: What was stopping them was that --inspect-brk unfortunately didn't exist yet when they started to work on the integration. So they couldn't do the "right" thing.

@refack

This comment has been minimized.

Copy link
Member Author

commented May 10, 2017

If I understand correctly CTC agreed to restore --inspect --debug-brk as alias to --inspect-brk, pending @ChALkeR's investigating an issue.

@Trott

This comment has been minimized.

Copy link
Member

commented May 14, 2017

Pinging @ChALkeR: Did you gather the information you needed to gather? Are we prepared to move forward with restoring --inspect --debug-brk? Or not yet?

@joshgav

This comment has been minimized.

Copy link
Member

commented May 15, 2017

@refack

CTC agreed to restore --inspect --debug-brk as alias to --inspect-brk, pending @ChALkeR's investigating an issue.

This is my understanding as well, and #12949 accomplishes that (and more).

@jkrems

going back to --debug (or --debug-brk w/o --inspect) anytime soon is pretty evil. The same command line option combination shouldn't trigger 2 completely different protocols across different versions of node.

I agree, and in fact I think this might be @ChALkeR's primary concern.

Based on this, #12949 should not add back --debug at all.

On the other hand, it could be helpful to our users to print a friendly message when --debug is specified in 8.x. I don't think that should be part of #12949 but would be appropriate for another PR.

refack added a commit to refack/node that referenced this issue May 29, 2017

inspector: --debug* deprecation and invalidation
PR-URL: nodejs#12949
Fixes: nodejs#12364
Fixes: nodejs#12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>

jasnell added a commit that referenced this issue May 29, 2017

inspector: --debug* deprecation and invalidation
PR-URL: #12949
Fixes: #12364
Fixes: #12630
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.