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

Global flags are not all global #2361

Closed
polinasok opened this issue Feb 24, 2021 · 5 comments · Fixed by #3395
Closed

Global flags are not all global #2361

polinasok opened this issue Feb 24, 2021 · 5 comments · Fixed by #3395

Comments

@polinasok
Copy link
Collaborator

polinasok commented Feb 24, 2021

I am noticing this in the context of dlv-dap, but this is true for other modes as well (e.g. build flags with exec, attach, etc). Also, as new flags get added, it is not clear if they are global and always apply as well.

It might be nice to have a map of subcommands and applicable flags and use that to set them. But if it is too tedious to add almost-global flags multiple times for the individual commands, then perhaps each such flag should mention in the description what subcommands it doesn't apply to? Or maybe each subcommand that ignores it should print a warning that the flag is ignored (either because the feature is not yet supported or because it doesn't make sense at all)?

In particular, based on dlv dap --help:

Global Flags:

  • --accept-multiclient
    • not yet supported
    • if set by user, prints "Warning: accept-multiclient mode not supported with dap"
  • --allow-non-terminal-interactive
    • new flag that was not there when I added dapCmd
    • not applicable? print warning if set?
  • --api-version int
    • doesn't apply with dap
    • does a warning make sense given that there is a default, so it will always be printed?
    • add a note to the flag description that version is meaningless under dap?
  • --backend
    • passed to debugger.Config
    • default is not empty, so support via flags instead of launch/attach args
    • does it make more sense to support this via launch/attach args instead or also?
    • in that case, update the flag description/warning?
  • --build-flags
    • not supported
    • If set, prints "Warning: build flags ignored with dap; specify via launch/attach request instead"
  • --check-go-version
    • passed to debugger.Config
    • default is not unset, so support via flags instead of launch/attach args
  • --disable-aslr Disables address space randomization
    • new flag added after dapCmd
    • pass to debugger.Config?
  • --headless
    • dap is always headless, so this flag doesn't make sense
    • if set, "Warning: dap mode is always headless"
    • or should we always require it to be set explicitly because one day we might want to add a terminal frontend?
  • --init string Init file, executed by the terminal client.
    • "Warning: init file ignored with dap"
  • --listen string Debugging server listen address. (default "127.0.0.1:0")
    • in use
  • --log
    • in use
  • --log-dest string
    • in use
  • --log-output string
    • in use
  • --only-same-user
    • new flag added after dapCmd
    • how to handle?
  • --redirect
    • new flag added to dapCmd
    • pass to debugger.Config?
  • --wd
    • supported via launch args
    • if set, prints "Warning: working directory ignored with dap"
@polinasok
Copy link
Collaborator Author

/cc @suzmue

@aarzilli
Copy link
Member

For the record we can't move those flags to subcommands because it would be backwards incompatible. For example dlv --headless debug ... would no longer be valid.

@polinasok
Copy link
Collaborator Author

Sure makes sense. It would be nice to be more consistent for new flags, but the old ones are what they are, minus the clarity of when they are applicable, which can still be addressed with warnings, better flag descriptions, additional notes under --help.

@aarzilli
Copy link
Member

aarzilli commented Dec 8, 2021

Should this stay open?

@derekparker
Copy link
Member

Perhaps we keep this open until we've improved warnings on when flags won't actually be used? Might be worth to create a checklist in this issue to keep track (cc @polinasok).

aarzilli added a commit to aarzilli/delve that referenced this issue May 28, 2023
Due to some very old mistakes too many of Delve's flags are declared as
persistent on cobra's root command. For example the headless flag is a
global flag but does not apply to connect, dap or trace; the backend
flag does not apply to replay, core and dap; etc.

Almost all global flags should have been declared as local flags on
individual subcommands. Unfortunately we can not change this without
breaking backwards compatibility, for example:

   dlv --headless debug

would not parse if headless was a flag of debug instead of a global
flag.

Instead we alter usage function and the markdown generation script to
strategically hide the flags that don't apply.

Fixes go-delve#2361
aarzilli added a commit to aarzilli/delve that referenced this issue Jul 20, 2023
Due to some very old mistakes too many of Delve's flags are declared as
persistent on cobra's root command. For example the headless flag is a
global flag but does not apply to connect, dap or trace; the backend
flag does not apply to replay, core and dap; etc.

Almost all global flags should have been declared as local flags on
individual subcommands. Unfortunately we can not change this without
breaking backwards compatibility, for example:

   dlv --headless debug

would not parse if headless was a flag of debug instead of a global
flag.

Instead we alter usage function and the markdown generation script to
strategically hide the flags that don't apply.

Fixes go-delve#2361
aarzilli added a commit to aarzilli/delve that referenced this issue Aug 1, 2023
Due to some very old mistakes too many of Delve's flags are declared as
persistent on cobra's root command. For example the headless flag is a
global flag but does not apply to connect, dap or trace; the backend
flag does not apply to replay, core and dap; etc.

Almost all global flags should have been declared as local flags on
individual subcommands. Unfortunately we can not change this without
breaking backwards compatibility, for example:

   dlv --headless debug

would not parse if headless was a flag of debug instead of a global
flag.

Instead we alter usage function and the markdown generation script to
strategically hide the flags that don't apply.

Fixes go-delve#2361
derekparker pushed a commit that referenced this issue Aug 9, 2023
Due to some very old mistakes too many of Delve's flags are declared as
persistent on cobra's root command. For example the headless flag is a
global flag but does not apply to connect, dap or trace; the backend
flag does not apply to replay, core and dap; etc.

Almost all global flags should have been declared as local flags on
individual subcommands. Unfortunately we can not change this without
breaking backwards compatibility, for example:

   dlv --headless debug

would not parse if headless was a flag of debug instead of a global
flag.

Instead we alter usage function and the markdown generation script to
strategically hide the flags that don't apply.

Fixes #2361
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants