Repository: microsoft/debugpy

Issue:
Terminal debug experience in Python Debbuger Extension
We are implemented a debug terminal with shell integration, in order to do that we will need an extra implementation in debugpy side.

In order to start debugging from the terminal, we need to specify a connect or listen port in the command: `python -m debugpy --connect 5678 file.py`.

What we want is to be able to use only: `python -m debugpy file.py` and debugpy set internally the connect port by default to 5678.


Discussion:

Patch:
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
index 9d2bead8..b0121d12 100644
--- a/CONTRIBUTING.md
+++ b/CONTRIBUTING.md
@@ -74,9 +74,24 @@ On Linux or macOS:
 .../debugpy$ python3 -m tox -e py27,py37 --develop
 ```
 
+You can run all tests in a single file using a specified python version, like this:
+```
+...\debugpy> py -m tox --develop -e py312 -- ".\tests\debugpy\server\test_cli.py"
+```
+
+You can also specify a single test, like this:
+```
+...\debugpy> py -m tox --develop -e py312 -- ".\tests\debugpy\server\test_cli.py::test_duplicate_switch"
+```
+
+The tests are run concurrently, and the default number of workers is 8. You can force a single worker by using the `-n0` flag, like this:
+```
+...\debugpy> py -m tox --develop -e py312 -- -n0 ".\tests\debugpy\server\test_cli.py"
+```
+
 ### Running tests without tox
 
-While tox is the recommended way to run the test suite, pytest can also be invoked directly from the root of the repository. This requires packages in tests/test_requirements.txt to be installed first.
+While tox is the recommended way to run the test suite, pytest can also be invoked directly from the root of the repository. This requires packages in tests/requirements.txt to be installed first.
 
 ## Using modified debugpy in Visual Studio Code
 To test integration between debugpy and Visual Studio Code, the latter can be directed to use a custom version of debugpy in lieu of the one bundled with the Python extension. This is done by specifying `"debugAdapterPath"` in `launch.json` - it must point at the root directory of the *package*, which is `src/debugpy` inside the repository:
diff --git a/src/debugpy/server/cli.py b/src/debugpy/server/cli.py
index a9c442f9..ae04ddba 100644
--- a/src/debugpy/server/cli.py
+++ b/src/debugpy/server/cli.py
@@ -7,10 +7,7 @@ import os
 import re
 import sys
 from importlib.util import find_spec
-from typing import Any
-from typing import Union
-from typing import Tuple
-from typing import Dict
+from typing import Any, Union, Tuple, Dict
 
 # debugpy.__main__ should have preloaded pydevd properly before importing this module.
 # Otherwise, some stdlib modules above might have had imported threading before pydevd
@@ -161,6 +158,7 @@ def set_target(kind: str, parser=(lambda x: x), positional=False):
                     import locale
 
                     target = target.decode(locale.getpreferredencoding(False))
+
         options.target = target
 
     return do
@@ -193,23 +191,68 @@ switches = [
 ]
 # fmt: on
 
-
+# Consume all the args from argv
 def consume_argv():
     while len(sys.argv) >= 2:
         value = sys.argv[1]
         del sys.argv[1]
         yield value
 
+# Consume all the args from a given list
+def consume_args(args: list):
+    if (args is sys.argv):
+        yield from consume_argv()
+    else:
+        while args:
+            value = args[0]
+            del args[0]
+            yield value
 
-def parse_argv():
+# Parse the args from the command line, then from the environment.
+# Args from the environment are only used if they are not already set from the command line.
+def parse_args():
+
+    # keep track of the switches we've seen so far
     seen = set()
-    it = consume_argv()
+
+    parse_args_from_command_line(seen)
+    parse_args_from_environment(seen)
+
+    # if the target is not set, or is empty, this is an error
+    if options.target is None or options.target == "":
+        raise ValueError("missing target: " + TARGET)
+
+    if options.mode is None:
+        raise ValueError("either --listen or --connect is required")
+    if options.adapter_access_token is not None and options.mode != "connect":
+        raise ValueError("--adapter-access-token requires --connect")
+    if options.target_kind == "pid" and options.wait_for_client:
+        raise ValueError("--pid does not support --wait-for-client")
+
+    assert options.target_kind is not None
+    assert options.address is not None
+
+def parse_args_from_command_line(seen: set):
+    parse_args_helper(sys.argv, seen)
+
+def parse_args_from_environment(seenFromCommandLine: set):
+    args = os.environ.get("DEBUGPY_EXTRA_ARGV")
+    if (not args):
+        return
+
+    argsList = args.split()
+
+    seenFromEnvironment = set()
+    parse_args_helper(argsList, seenFromCommandLine, seenFromEnvironment, True)
+
+def parse_args_helper(args: list, seenFromCommandLine: set, seenFromEnvironment: set = set(), isFromEnvironment=False):
+    iterator = consume_args(args)
 
     while True:
         try:
-            arg = next(it)
+            arg = next(iterator)
         except StopIteration:
-            raise ValueError("missing target: " + TARGET)
+            break
 
         switch = arg
         if not switch.startswith("-"):
@@ -220,34 +263,37 @@ def parse_argv():
         else:
             raise ValueError("unrecognized switch " + switch)
 
-        if switch in seen:
-            raise ValueError("duplicate switch " + switch)
+        # if we're parsing from the command line, and we've already seen the switch on the command line, this is an error
+        if (not isFromEnvironment and switch in seenFromCommandLine):
+            raise ValueError("duplicate switch on command line: " + switch)
+        # if we're parsing from the environment, and we've already seen the switch in the environment, this is an error
+        elif (isFromEnvironment and switch in seenFromEnvironment):
+            raise ValueError("duplicate switch from environment: " + switch)
+        # if we're parsing from the environment, and we've already seen the switch on the command line, skip it, since command line takes precedence
+        elif (isFromEnvironment and switch in seenFromCommandLine):
+            continue
+        # otherwise, the switch is new, so add it to the appropriate set
         else:
-            seen.add(switch)
+            if (isFromEnvironment):
+                seenFromEnvironment.add(switch)
+            else:
+                seenFromCommandLine.add(switch)
 
+        # process the switch, running the corresponding action
         try:
-            action(arg, it)
+            action(arg, iterator)
         except StopIteration:
             assert placeholder is not None
             raise ValueError("{0}: missing {1}".format(switch, placeholder))
         except Exception as exc:
             raise ValueError("invalid {0} {1}: {2}".format(switch, placeholder, exc))
 
-        if options.target is not None:
+        # If we're parsing the command line, we're done after we've processed the target
+        # Otherwise, we need to keep parsing until all args are consumed, since the target may be set from the command line
+        # already, but there might be additional args in the environment that we want to process.
+        if (not isFromEnvironment and options.target is not None):
             break
 
-    if options.mode is None:
-        raise ValueError("either --listen or --connect is required")
-    if options.adapter_access_token is not None and options.mode != "connect":
-        raise ValueError("--adapter-access-token requires --connect")
-    if options.target_kind == "pid" and options.wait_for_client:
-        raise ValueError("--pid does not support --wait-for-client")
-
-    assert options.target is not None
-    assert options.target_kind is not None
-    assert options.address is not None
-
-
 def start_debugging(argv_0):
     # We need to set up sys.argv[0] before invoking either listen() or connect(),
     # because they use it to report the "process" event. Thus, we can't rely on
@@ -411,7 +457,7 @@ attach_pid_injected.attach(setup);
 def main():
     original_argv = list(sys.argv)
     try:
-        parse_argv()
+        parse_args()
     except Exception as exc:
         print(str(HELP) + str("\nError: ") + str(exc), file=sys.stderr)
         sys.exit(2)


Test patch:
diff --git a/tests/debugpy/server/test_cli.py b/tests/debugpy/server/test_cli.py
index 6b86c6b1..141d3266 100644
--- a/tests/debugpy/server/test_cli.py
+++ b/tests/debugpy/server/test_cli.py
@@ -2,11 +2,16 @@
 # Licensed under the MIT License. See LICENSE in the project root
 # for license information.
 
+import os
 import pickle
 import pytest
 import subprocess
 import sys
 
+# This is used for mocking environment variables
+# See https://docs.python.org/3/library/unittest.mock-examples.html for more info
+from unittest import mock
+
 from debugpy.common import log
 from tests.patterns import some
 
@@ -21,7 +26,7 @@ def cli(pyfile):
         from debugpy.server import cli
 
         try:
-            cli.parse_argv()
+            cli.parse_args()
         except Exception as exc:
             os.write(1, pickle.dumps(exc))
             sys.exit(1)
@@ -43,14 +48,19 @@ def cli(pyfile):
                 "wait_for_client",
             ]
         }
+
+        # Serialize the command line args and the options to stdout
         os.write(1, pickle.dumps([sys.argv[1:], options]))
 
     def parse(args):
         log.debug("Parsing argv: {0!r}", args)
         try:
+            # Run the CLI parser in a subprocess, and capture its output.
             output = subprocess.check_output(
                 [sys.executable, "-u", cli_parser.strpath] + args
             )
+
+            # Deserialize the output and return the parsed argv and options.
             argv, options = pickle.loads(output)
         except subprocess.CalledProcessError as exc:
             raise pickle.loads(exc.output)
@@ -62,6 +72,7 @@ def cli(pyfile):
     return parse
 
 
+# Test a combination of command line switches
 @pytest.mark.parametrize("target_kind", ["file", "module", "code"])
 @pytest.mark.parametrize("mode", ["listen", "connect"])
 @pytest.mark.parametrize("address", ["8888", "localhost:8888"])
@@ -71,7 +82,7 @@ def test_targets(cli, target_kind, mode, address, wait_for_client, script_args):
     expected_options = {
         "mode": mode,
         "target_kind": target_kind,
-        "wait_for_client": bool(wait_for_client),
+        "wait_for_client": False
     }
 
     args = ["--" + mode, address]
@@ -84,6 +95,7 @@ def test_targets(cli, target_kind, mode, address, wait_for_client, script_args):
 
     if wait_for_client:
         args += ["--wait-for-client"]
+        expected_options["wait_for_client"] = True
 
     if target_kind == "file":
         target = "spam.py"
@@ -119,32 +131,98 @@ def test_targets(cli, target_kind, mode, address, wait_for_client, script_args):
     assert argv == script_args
     assert options == some.dict.containing(expected_options)
 
-
-@pytest.mark.parametrize("value", ["", True, False])
+@pytest.mark.parametrize("value", [True, False])
 def test_configure_subProcess(cli, value):
-    args = ["--listen", "8888"]
-
-    if value == "":
-        value = True
-    else:
-        args += ["--configure-subProcess", str(value)]
-
-    args += ["spam.py"]
+    args = ["--listen", "8888", "--configure-subProcess", str(value), "spam.py"]
     _, options = cli(args)
 
     assert options["config"]["subProcess"] == value
 
+@pytest.mark.parametrize("value", [True, False])
+def test_configure_subProcess_from_environment(cli, value):
+    args = ["--listen", "8888", "spam.py"]
+    with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--configure-subProcess " + str(value)}):
+        _, options = cli(args)
+
+        assert options["config"]["subProcess"] == value
 
 def test_unsupported_switch(cli):
-    with pytest.raises(Exception):
+    with pytest.raises(ValueError) as ex:
         cli(["--listen", "8888", "--xyz", "123", "spam.py"])
+    
+    assert "unrecognized switch --xyz" in str(ex.value)
 
+def test_unsupported_switch_from_environment(cli):
+    with pytest.raises(ValueError) as ex:
+        with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--xyz 123"}):
+            cli(["--listen", "8888", "spam.py"])
+    
+    assert "unrecognized switch --xyz" in str(ex.value)
 
 def test_unsupported_configure(cli):
-    with pytest.raises(Exception):
+    with pytest.raises(ValueError) as ex:
         cli(["--connect", "127.0.0.1:8888", "--configure-xyz", "123", "spam.py"])
+    
+    assert "unknown property 'xyz'" in str(ex.value)
+
+def test_unsupported_configure_from_environment(cli):
+    with pytest.raises(ValueError) as ex:
+        with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--configure-xyz 123"}):
+            cli(["--connect", "127.0.0.1:8888", "spam.py"])
 
+    assert "unknown property 'xyz'" in str(ex.value)
 
 def test_address_required(cli):
-    with pytest.raises(Exception):
+    with pytest.raises(ValueError) as ex:
         cli(["-m", "spam"])
+    
+    assert "either --listen or --connect is required" in str(ex.value)
+
+def test_missing_target(cli):
+    with pytest.raises(ValueError) as ex:
+        cli(["--listen", "8888"])
+    
+    assert "missing target" in str(ex.value)
+
+def test_duplicate_switch(cli):
+    with pytest.raises(ValueError) as ex:
+        cli(["--listen", "8888", "--listen", "9999", "spam.py"])
+    
+    assert "duplicate switch on command line: --listen" in str(ex.value)
+
+def test_duplicate_switch_from_environment(cli):
+    with pytest.raises(ValueError) as ex:
+        with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--listen 8888 --listen 9999"}):
+            cli(["spam.py"])
+    
+    assert "duplicate switch from environment: --listen" in str(ex.value)
+
+# Test that switches can be read from the environment
+def test_read_switches_from_environment(cli):
+    args = ["spam.py"]
+
+    with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--connect 5678"}):
+        _, options = cli(args)
+
+        assert options["mode"] == "connect"
+        assert options["address"] == ("127.0.0.1", 5678)
+        assert options["target"] == "spam.py"
+
+# Test that command line switches override environment variables
+def test_override_environment_switch(cli):
+    args = ["--connect", "8888", "spam.py"]
+
+    with mock.patch.dict(os.environ, {"DEBUGPY_EXTRA_ARGV": "--connect 5678"}):
+        _, options = cli(args)
+
+        assert options["mode"] == "connect"
+        assert options["address"] == ("127.0.0.1", 8888)
+        assert options["target"] == "spam.py"
+
+# Test that script args (passed to target) are preserved
+def test_script_args(cli):
+    args = ["--listen", "8888", "spam.py", "arg1", "arg2"]
+    argv, options = cli(args)
+
+    assert argv == ["arg1", "arg2"]
+    assert options["target"] == "spam.py"
\ No newline at end of file