Merge pull request #1624 from microsoft/extra_argv_envvar

Add parsing args from environment

Author: AdamYoblick

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:

src/debugpy/server/cli.py

@@ -7,10 +7,7 @@
 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 do(arg, it):
                     import locale
 
                     target = target.decode(locale.getpreferredencoding(False))
+
         options.target = target
 
     return do
@@ -193,23 +191,68 @@ def do(arg, it):
 ]
 # 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 @@ def attach_to_pid():
 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)

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_parser():
         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_parser():
                 "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 parse(args):
     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"