diff --git a/.chronus/changes/fix-keep-setup-py-coercion-2026-4-30-5-30-0.md b/.chronus/changes/fix-keep-setup-py-coercion-2026-4-30-5-30-0.md new file mode 100644 index 00000000000..dfa122dd7f1 --- /dev/null +++ b/.chronus/changes/fix-keep-setup-py-coercion-2026-4-30-5-30-0.md @@ -0,0 +1,7 @@ +--- +changeKind: internal +packages: + - "@typespec/http-client-python" +--- + +Fix `run_batch.py` passing string `"true"`/`"false"` values to pygen, which caused `keep-setup-py="false"` to be treated as truthy and generate `setup.py` instead of `pyproject.toml` for all regenerated test packages. diff --git a/packages/http-client-python/emitter/src/types.ts b/packages/http-client-python/emitter/src/types.ts index c8dbcf9b584..2d9bd5361ad 100644 --- a/packages/http-client-python/emitter/src/types.ts +++ b/packages/http-client-python/emitter/src/types.ts @@ -545,6 +545,22 @@ export const KnownTypes = { any: { type: "any" }, }; +/** + * Detect whether an endpoint template parameter should be treated as an API + * version parameter even when TCGC does not flag it as such. This is a + * compatibility shim for unreleased TCGC changes that gate the name-based + * heuristic on `isMetadata`, which excludes server template parameters. + */ +function isEndpointApiVersionFallback( + param: { name: string; isApiVersionParam: boolean }, + serviceApiVersions: string[], +): boolean { + if (param.isApiVersionParam) return false; + if (serviceApiVersions.length === 0) return false; + const lower = param.name.toLowerCase(); + return lower === "apiversion" || lower === "api-version"; +} + export function emitEndpointType( context: PythonSdkContext, type: SdkEndpointType, @@ -554,13 +570,29 @@ export function emitEndpointType( for (const param of type.templateArguments) { const paramBase = emitParamBase(context, param, undefined, serviceApiVersions); paramBase.clientName = context.arm ? "base_url" : paramBase.clientName; + + let effectiveClientDefaultValue = param.clientDefaultValue; + // If this endpoint template param looks like an api-version but TCGC + // did not flag it, apply fallback: mark as api version and derive defaults. + if (isEndpointApiVersionFallback(param, serviceApiVersions)) { + paramBase.isApiVersion = true; + if (!effectiveClientDefaultValue) { + effectiveClientDefaultValue = serviceApiVersions[serviceApiVersions.length - 1]; + } + paramBase.type = getSimpleTypeResult(context, { + type: "constant", + value: effectiveClientDefaultValue, + valueType: paramBase.type, + }); + } + params.push({ ...paramBase, - optional: Boolean(param.clientDefaultValue), + optional: Boolean(effectiveClientDefaultValue), wireName: param.name, location: "endpointPath", implementation: getImplementation(context, param), - clientDefaultValue: param.clientDefaultValue, + clientDefaultValue: effectiveClientDefaultValue, skipUrlEncoding: param.allowReserved, }); context.__endpointPathParameters!.push(params.at(-1)!); diff --git a/packages/http-client-python/eng/scripts/ci/run-tests.ts b/packages/http-client-python/eng/scripts/ci/run-tests.ts index 969ff746346..dbd27fc7b1a 100644 --- a/packages/http-client-python/eng/scripts/ci/run-tests.ts +++ b/packages/http-client-python/eng/scripts/ci/run-tests.ts @@ -80,6 +80,7 @@ interface ToxResult { success: boolean; duration: number; error?: string; + failedTests?: string[]; } interface RunningTask { @@ -88,6 +89,29 @@ interface RunningTask { promise: Promise; } +function extractFailedTests(output: string): string[] { + const lines = output.split("\n"); + const failedTests: string[] = []; + let inSummary = false; + + for (const line of lines) { + if (line.includes("short test summary info")) { + inSummary = true; + continue; + } + if (inSummary) { + const match = line.match(/^FAILED\s+(.+)/); + if (match) { + failedTests.push(match[1].trim()); + } else if (line.match(/^={3,}/) || line.match(/^\d+ failed/)) { + break; + } + } + } + + return failedTests; +} + function startToxEnv(env: string, pythonPath: string, name?: string): RunningTask { const startTime = Date.now(); const toxIniPath = join(testsDir, "tox.ini"); @@ -102,14 +126,28 @@ function startToxEnv(env: string, pythonPath: string, name?: string): RunningTas const proc: ChildProcess = spawn(pythonPath, args, { cwd: testsDir, - stdio: !argv.values.quiet ? "inherit" : "pipe", + stdio: "pipe", env: { ...process.env, FOLDER: env.split("-")[1] || "azure" }, }); let stderr = ""; - if (argv.values.quiet && proc.stderr) { + let stdout = ""; + if (proc.stdout) { + proc.stdout.on("data", (data) => { + const chunk = data.toString(); + stdout += chunk; + if (!argv.values.quiet) { + process.stdout.write(chunk); + } + }); + } + if (proc.stderr) { proc.stderr.on("data", (data) => { - stderr += data.toString(); + const chunk = data.toString(); + stderr += chunk; + if (!argv.values.quiet) { + process.stderr.write(chunk); + } }); } @@ -124,11 +162,14 @@ function startToxEnv(env: string, pythonPath: string, name?: string): RunningTas console.log(`${pc.red("[FAIL]")} ${env} (${duration.toFixed(1)}s)`); } + const failedTests = extractFailedTests(stdout); + resolve({ env, success, duration, error: success ? undefined : stderr || `Exit code: ${code}`, + failedTests: failedTests.length > 0 ? failedTests : undefined, }); }); @@ -218,7 +259,8 @@ async function _runSequential( ): Promise { const results: ToxResult[] = []; for (const env of envs) { - const result = await runToxEnv(env, pythonPath, name); + const task = startToxEnv(env, pythonPath, name); + const result = await task.promise; results.push(result); } return results; @@ -251,7 +293,11 @@ function printSummary(results: ToxResult[]): void { console.log(pc.red("Failed environments:")); for (const result of failed) { console.log(` - ${result.env}`); - if (result.error && !argv.values.quiet) { + if (result.failedTests && result.failedTests.length > 0) { + for (const test of result.failedTests) { + console.log(` ${pc.red("FAILED")} ${test}`); + } + } else if (result.error && !argv.values.quiet) { console.log(` ${result.error.split("\n").slice(0, 5).join("\n ")}`); } } diff --git a/packages/http-client-python/eng/scripts/setup/run_batch.py b/packages/http-client-python/eng/scripts/setup/run_batch.py index 9548962b417..6d6bef5b0d2 100644 --- a/packages/http-client-python/eng/scripts/setup/run_batch.py +++ b/packages/http-client-python/eng/scripts/setup/run_batch.py @@ -39,8 +39,21 @@ def process_single_spec(config_path_str: str) -> tuple[str, bool, str]: output_dir = config["outputDir"] # Pass command args directly to pygen - pygen expects hyphenated keys - # Remove keys that shouldn't be passed to pygen - pygen_args = {k: v for k, v in command_args.items() if k not in ["emit-yaml-only"]} + # Remove keys that shouldn't be passed to pygen. + # Also coerce the string values "true"/"false" to real booleans, matching the behavior + # of pygen.utils.parse_args (the CLI path). Without this, the emitter passes string + # "false" for flags like keep-setup-py, which is truthy in Python and causes pygen to + # take the wrong branch (e.g. generating setup.py instead of pyproject.toml). + def _coerce(value): + if value == "true": + return True + if value == "false": + return False + return value + + pygen_args = { + k: _coerce(v) for k, v in command_args.items() if k not in ["emit-yaml-only"] + } # Run preprocess and codegen (black is batched at the end for performance) preprocess.PreProcessPlugin(output_folder=output_dir, tsp_file=yaml_path, **pygen_args).process() diff --git a/packages/http-client-python/generator/pygen/codegen/models/parameter.py b/packages/http-client-python/generator/pygen/codegen/models/parameter.py index 26eb0ba5c9a..08904bef36c 100644 --- a/packages/http-client-python/generator/pygen/codegen/models/parameter.py +++ b/packages/http-client-python/generator/pygen/codegen/models/parameter.py @@ -377,6 +377,8 @@ def is_host(self) -> bool: def method_location(self) -> ParameterMethodLocation: if self.constant: return ParameterMethodLocation.KWARG + if self.is_api_version: + return ParameterMethodLocation.KWARG if ( self.is_host and (self.code_model.options["version-tolerant"] or self.code_model.options["low-level-client"]) @@ -408,6 +410,6 @@ def is_host(self) -> bool: @property def method_location(self) -> ParameterMethodLocation: - if self.constant: + if self.constant or self.is_api_version: return ParameterMethodLocation.KWARG return ParameterMethodLocation.POSITIONAL diff --git a/packages/http-client-python/generator/pygen/codegen/serializers/__init__.py b/packages/http-client-python/generator/pygen/codegen/serializers/__init__.py index 37657b08b29..73525287147 100644 --- a/packages/http-client-python/generator/pygen/codegen/serializers/__init__.py +++ b/packages/http-client-python/generator/pygen/codegen/serializers/__init__.py @@ -42,15 +42,15 @@ "JinjaSerializer", ] -_PACKAGE_FILES = [ +_DEFAULT_PACKAGE_FILES = ( "CHANGELOG.md.jinja2", "dev_requirements.txt.jinja2", "LICENSE.jinja2", "MANIFEST.in.jinja2", "README.md.jinja2", -] +) -_REGENERATE_FILES = {"MANIFEST.in"} +_DEFAULT_REGENERATE_FILES = frozenset({"MANIFEST.in"}) AsyncInfo = namedtuple("AsyncInfo", ["async_mode", "async_path"]) @@ -75,15 +75,17 @@ def __init__( ) -> None: super().__init__(output_folder=output_folder, **kwargs) self.code_model = code_model + self._package_files: list[str] = list(_DEFAULT_PACKAGE_FILES) + self._regenerate_files: set[str] = set(_DEFAULT_REGENERATE_FILES) self._regenerate_setup_py() def _regenerate_setup_py(self): if self.code_model.options["keep-setup-py"] or self.code_model.options["basic-setup-py"]: - _PACKAGE_FILES.append("setup.py.jinja2") - _REGENERATE_FILES.add("setup.py") + self._package_files.append("setup.py.jinja2") + self._regenerate_files.add("setup.py") else: - _PACKAGE_FILES.append("pyproject.toml.jinja2") - _REGENERATE_FILES.add("pyproject.toml") + self._package_files.append("pyproject.toml.jinja2") + self._regenerate_files.add("pyproject.toml") @property def has_aio_folder(self) -> bool: @@ -253,7 +255,7 @@ def _serialize_and_write_package_files(self) -> None: lstrip_blocks=True, ) - package_files = list(_PACKAGE_FILES) # Copy to avoid modifying global + package_files = list(self._package_files) if not self.code_model.license_description: package_files.remove("LICENSE.jinja2") elif Path(self.code_model.options["package-mode"]).exists(): @@ -272,7 +274,7 @@ def _serialize_and_write_package_files(self) -> None: continue file = template_name.replace(".jinja2", "") output_file = root_of_sdk / file - if not self.read_file(output_file) or file in _REGENERATE_FILES: + if not self.read_file(output_file) or file in self._regenerate_files: if self.keep_version_file and file == "setup.py" and not self.code_model.options["azure-arm"]: # don't regenerate setup.py file if the version file is more up to date for data-plane continue diff --git a/packages/http-client-python/generator/pygen/codegen/serializers/parameter_serializer.py b/packages/http-client-python/generator/pygen/codegen/serializers/parameter_serializer.py index 9091f86748d..00928ec39f2 100644 --- a/packages/http-client-python/generator/pygen/codegen/serializers/parameter_serializer.py +++ b/packages/http-client-python/generator/pygen/codegen/serializers/parameter_serializer.py @@ -202,11 +202,13 @@ def append_pop_kwarg(key: str, pop_type: PopKwargType) -> None: if is_content_type_optional and not type_annotation.startswith("Optional[") else type_annotation ) - if kwarg.client_default_value is not None or kwarg.optional or kwarg.constant: + if kwarg.client_default_value is not None or kwarg.optional or kwarg.constant or kwarg.is_api_version: if check_client_input and kwarg.check_client_input: default_value = f"self._config.{kwarg.client_name}" elif kwarg.constant: default_value = kwarg.type.get_declaration(None) + elif kwarg.is_api_version and kwarg.client_default_value is None and kwarg.api_versions: + default_value = f'"{kwarg.api_versions[-1]}"' else: default_value = kwarg.client_default_value_declaration if check_kwarg_dict and (kwarg.location in [ParameterLocation.HEADER, ParameterLocation.QUERY]): diff --git a/packages/http-client-python/tests/unit/test_generated_packaging_files.py b/packages/http-client-python/tests/unit/test_generated_packaging_files.py new file mode 100644 index 00000000000..6e77b5d43b8 --- /dev/null +++ b/packages/http-client-python/tests/unit/test_generated_packaging_files.py @@ -0,0 +1,65 @@ +# ------------------------------------------------------------------------- +# Copyright (c) Microsoft Corporation. All rights reserved. +# Licensed under the MIT License. See License.txt in the project root for +# license information. +# -------------------------------------------------------------------------- +"""Verify regenerated test packages emit pyproject.toml, not setup.py. + +The Python emitter passes options to pygen via JSON config files. Boolean +flags arrive as the strings "true"/"false". If those strings are not coerced +to real booleans before being unpacked as kwargs to pygen, a value like +keep-setup-py="false" is treated as truthy in Python and pygen emits +setup.py instead of pyproject.toml. This test guards against that +regression by walking the regenerated test fixtures. +""" +from pathlib import Path + +import pytest + +# Packages that intentionally opt into setup.py via keep-setup-py=true in +# eng/scripts/ci/regenerate.ts. They should have setup.py and no pyproject.toml. +KEEP_SETUP_PY_PACKAGES = {"setuppy-authentication-union"} + +_GENERATED_ROOT = Path(__file__).resolve().parents[1] / "generated" + + +def _iter_generated_packages(): + if not _GENERATED_ROOT.is_dir(): + return + for flavor_dir in sorted(_GENERATED_ROOT.iterdir()): + if not flavor_dir.is_dir(): + continue + for pkg_dir in sorted(flavor_dir.iterdir()): + if not pkg_dir.is_dir(): + continue + yield flavor_dir.name, pkg_dir + + +@pytest.mark.parametrize( + "flavor,pkg_dir", + list(_iter_generated_packages()), + ids=lambda v: v.name if isinstance(v, Path) else v, +) +def test_generated_package_uses_pyproject_toml(flavor, pkg_dir): + has_setup_py = (pkg_dir / "setup.py").is_file() + has_pyproject = (pkg_dir / "pyproject.toml").is_file() + + if pkg_dir.name in KEEP_SETUP_PY_PACKAGES: + assert has_setup_py, f"{flavor}/{pkg_dir.name} should have setup.py (keep-setup-py=true)" + assert not has_pyproject, ( + f"{flavor}/{pkg_dir.name} should NOT have pyproject.toml (keep-setup-py=true)" + ) + return + + assert has_pyproject, ( + f"{flavor}/{pkg_dir.name} is missing pyproject.toml. " + f"This usually means string boolean args (e.g. keep-setup-py=\"false\") " + f"were not coerced to real booleans before being passed to pygen. " + f"See eng/scripts/setup/run_batch.py." + ) + assert not has_setup_py, ( + f"{flavor}/{pkg_dir.name} has setup.py but should use pyproject.toml. " + f"This usually means string boolean args (e.g. keep-setup-py=\"false\") " + f"were not coerced to real booleans before being passed to pygen. " + f"See eng/scripts/setup/run_batch.py." + )