fix: use pyproject.toml metadata when sdist has no setup.py or PKG-INFO - #920
fix: use pyproject.toml metadata when sdist has no setup.py or PKG-INFO#920Sanjays2402 wants to merge 4 commits into
Conversation
![aws-sam-tooling-bot[bot]](https://avatars.githubusercontent.com/b/1290?s=60&v=4){kind=small}
| if not in_project_section or stripped.startswith("#") or "=" not in stripped: | ||
| continue | ||
| key, _, value = stripped.partition("=") | ||
| value = value.strip().strip("\"'") |
There was a problem hiding this comment.
[BUG] The line-based fallback parser can silently return a corrupted version, and this path is live in production: setup.py declares python_requires=">=3.10", and Python 3.10 has no stdlib tomllib, so it is the only parse path there.
Two concrete defects:
- Inline comments are not stripped.
value.strip().strip("\"'")only removes quote characters at the ends of the string, so:
[project]
name = "foo"
version = "1.2.3" # x-release-please-versionyields version == '1.2.3" # x-release-please-version' (the leading quote is stripped, the trailing one is not, because the last character is n). Version-bumping tools (release-please, bump-my-version, tbump) commonly add exactly this kind of trailing marker comment. The bad value propagates into Package.identifier ("%s==%s" % (name, version)) and data_dir, so the sdist no longer matches its built wheel during dependency reconciliation — a wrong-metadata failure that is harder to diagnose than the UnsupportedPackageError this PR is fixing.
- The table header comparison is exact (
stripped == "[project]"), so a valid header with a trailing comment ([project] # main table) leavesin_project_sectionfalse and the fallback reports "no metadata".
Suggested tightening — require a properly quoted scalar and tolerate comments:
QUOTEDVALUE = re.compile(r"""^(["'])(.*?)\1\s*(?:#.*)?$""")
...
if stripped.startswith("["):
in_project_section = stripped.split("#")[0].strip() == "[project]"
continue
...
key, , value = stripped.partition("=")
match = QUOTED_VALUE.match(value.strip())
if not match:
continue
value = match.group(2)Rejecting anything that is not a quoted scalar also avoids picking up array/inline-table values (dynamic = ["version"]-style lines) as a name or version.
There was a problem hiding this comment.
Fixed. The line-based parser now requires properly quoted scalars via a QUOTED_VALUE regex, so inline comments outside quotes are stripped, trailing comments on the [project] header are tolerated, and non-scalar values like dynamic = ["version"] are skipped instead of being picked up. Added tests covering the release-please trailing-comment version, a commented header, and array-value rejection — all passing.
| candidate_name, candidate_version = project.get("name"), project.get("version") | ||
| if isinstance(candidate_name, str) and isinstance(candidate_version, str): | ||
| return candidate_name, candidate_version | ||
| except Exception: |
There was a problem hiding this comment.
[ERROR_HANDLING] except Exception: pass here is broader than intended and produces no diagnostic. Two consequences:
- Real bugs (an
AttributeErrorifprojectis not a table, an unexpected type error) are indistinguishable from a TOML syntax error, and nothing is logged, so the user only sees the eventualUnsupportedPackageErrorwith no clue why. The surrounding code consistently logs in this situation — e.g._get_pkg_info_filepathusesLOG.debug("Could not check setuptools availability: %s", e). - On Python 3.11+, a
tomllibfailure falls through to the hand-rolled parser, which will happily extract values from a file that a real TOML parser rejected as invalid. Accepting metadata from a malformedpyproject.tomlis worse than reporting the package as unsupported, since no PEP 517 backend would build it with those values.
Narrow the catch and gate the line-based parse on tomllib actually being unavailable:
if tomllib is not None:
try:
project = tomllib.loads(contents).get("project") or {}
except tomllib.TOMLDecodeError as ex:
LOG.debug("Unable to parse pyproject.toml: %s", ex)
return None, None
candidate_name, candidate_version = project.get("name"), project.get("version")
if isinstance(candidate_name, str) and isinstance(candidate_version, str):
return candidate_name, candidate_version
return None, NoneThat keeps the stdlib parser authoritative when it exists and leaves the line-based parse as the Python 3.10-only path it is documented to be.
There was a problem hiding this comment.
Done. The tomllib path now catches TOMLDecodeError specifically with a LOG.debug message and returns (None, None), and the hand-rolled parser only runs when tomllib is None — the documented 3.10-only path. Added a test asserting a malformed pyproject.toml yields (None, None) on 3.11+ rather than falling through to the line-based parse.
github-actions
Bot
added
pr/external
stage/needs-triage
- Line-based fallback (3.10-only): require quoted scalars via QUOTED_VALUE regex, tolerate trailing comments on the [project] header, strip inline comments outside quotes, skip non-scalar values like dynamic = ["version"] - Narrow except Exception to tomllib.TOMLDecodeError with LOG.debug; malformed TOML on 3.11+ now returns (None, None) instead of falling through to the hand-rolled parser - Add tests: trailing-comment version, commented header, non-scalar rejection, malformed-TOML-on-3.11+ behavior
| except tomllib.TOMLDecodeError as ex: | ||
| LOG.debug("Unable to parse pyproject.toml with tomllib: %s", ex) | ||
| return None, None | ||
| candidate_name, candidate_version = project.get("name"), project.get("version") |
There was a problem hiding this comment.
[ERROR_HANDLING] project is not guaranteed to be a table. A top-level project = "something" is valid TOML, so tomllib.loads(contents).get("project") can return a str (or any scalar/list), and the following project.get("name") raises AttributeError.
This was previously masked by the broad except Exception: pass; now that the except is correctly narrowed to TOMLDecodeError, the AttributeError escapes get_package_name_and_version and surfaces as a raw traceback in sam build instead of the actionable UnsupportedPackageError. Since the input is a third-party sdist, the parser should not assume shape — note the function already type-checks name/version with isinstance, so the table itself is the one unchecked value.
try:
parsed = tomllib.loads(contents)
except tomllib.TOMLDecodeError as ex:
LOG.debug("Unable to parse pyproject.toml with tomllib: %s", ex)
return None, None
project = parsed.get("project")
if not isinstance(project, dict):
return None, None
candidate_name, candidate_version = project.get("name"), project.get("version")There was a problem hiding this comment.
Fixed — the tomllib path now checks isinstance(project, dict) before touching .get, so a scalar like project = "something" returns (None, None) instead of raising AttributeError. Added a test for the non-table case.
| pyproject_path = self._osutils.joinpath(package_dir, "pyproject.toml") | ||
| if not self._osutils.file_exists(pyproject_path): | ||
| raise UnsupportedPackageError(self._osutils.basename(package_dir)) | ||
| contents = self._osutils.get_file_contents(pyproject_path, binary=False) |
There was a problem hiding this comment.
[ERROR_HANDLING] The file read is unguarded, so it can raise out of this "last resort" helper instead of degrading to UnsupportedPackageError. OSUtils.get_file_contents(..., binary=False) decodes with encoding="utf-8", so any sdist whose pyproject.toml is not valid UTF-8 (or has a UTF-8 BOM) raises UnicodeDecodeError. That exception propagates through get_package_name_and_version → Package._calculate_name_and_version, turning a package that previously failed with the clear "Unable to retrieve name/version for package" message into an unhandled internal error.
Using utf-8-sig also lets BOM-prefixed files parse successfully rather than failing in tomllib and silently discarding usable metadata:
try:
contents = self._osutils.get_file_contents(pyproject_path, binary=False, encoding="utf-8-sig")
except (OSError, UnicodeDecodeError) as ex:
LOG.debug("Unable to read %s: %s", pyproject_path, ex)
raise UnsupportedPackageError(self._osutils.basename(package_dir))There was a problem hiding this comment.
Done — the read now uses utf-8-sig (so BOM-prefixed files still parse) and is wrapped in try/except (OSError, UnicodeDecodeError) that logs at debug and raises UnsupportedPackageError instead of leaking the raw decode error. Added tests for a non-UTF-8 file and a BOM-prefixed file.
| # get the name and version from the result setup.py | ||
| pkg_info_filepath = self._get_pkg_info_filepath(package_dir) | ||
| name, version = self._get_name_version(pkg_info_filepath) | ||
| except UnsupportedPackageError: |
There was a problem hiding this comment.
[GENERAL] Recovering here leaves a misleading WARNING in the build output on the success path. _get_pkg_info_filepath emits LOG.warning("Unable to find PKG-INFO file for package in %s. This may be due to missing setuptools/distutils in Python 3.12+ or an incomplete sdist package.", ...) immediately before raising UnsupportedPackageError. That warning was previously followed by a hard failure, so it was accurate; after this change, every PEP 517-only git+https requirement — the exact case this PR fixes — will print it and then build successfully.
Users will reasonably read that as a broken build. Since _get_pkg_info_filepath now has a caller that treats the condition as recoverable, the "we could not find metadata here" message belongs at LOG.debug, with the warning (or nothing at all) decided by the caller once all fallbacks are exhausted. At minimum, log an explicit recovery message in this except branch so the preceding warning is not the last thing the user sees about this package.
There was a problem hiding this comment.
Agreed — demoted that warning to debug since the caller can now recover, and added an explicit LOG.info in the recovery branch so a successful pyproject fallback is visible without looking like a failure.
…ded read, demote misleading warning)
| raise UnsupportedPackageError(self._osutils.basename(package_dir)) from ex | ||
| name, version = _parse_pyproject_name_version(contents) | ||
| if not name or not version: | ||
| raise UnsupportedPackageError(self._osutils.basename(package_dir)) |
There was a problem hiding this comment.
[GENERAL] Downgrading the _get_pkg_info_filepath message to LOG.debug was correct for the recoverable case, but it leaves the terminal failure path with no user-visible diagnostic at all. Both remaining messages on that path are debug-level:
_get_pkg_info_filepath:LOG.debug("Unable to find PKG-INFO file ... missing setuptools/distutils in Python 3.12+ ...")- this method:
LOG.debug("Unable to read %s: %s", ...)
So when the pyproject fallback does not apply — no pyproject.toml, dynamic = ["version"], or metadata under [tool.poetry] instead of [project] — the user sees only UnsupportedPackageError: Unable to retrieve name/version for package: <dir>. That is exactly the opaque message reported in #675, and before this PR it was at least preceded by a WARNING naming the likely cause. Default sam build output does not include debug logs, so the actionable hint is lost.
Emitting the warning at the point where recovery definitively fails keeps the success path clean while preserving the diagnostic:
name, version = _parse_pyproject_name_version(contents)
if not name or not version:
LOG.warning(
"Unable to determine a static name/version for the package in %s. "
"No PKG-INFO metadata was available (this may be due to missing "
"setuptools/distutils in Python 3.12+) and pyproject.toml has no "
"static [project] name/version.",
package_dir,
)
raise UnsupportedPackageError(self._osutils.basename(package_dir))The same treatment would apply to the file_exists and read-failure branches above. Note that tests/unit/workflows/python_pip/test_packager.py::test_get_pkg_info_filepath_no_warning_when_missing stays valid, since it asserts no warning from _get_pkg_info_filepath specifically, not from this method.
There was a problem hiding this comment.
You're right, I overcorrected — the terminal path was left with no user-visible diagnostic at all. There's now a single WARNING emitted at the point where recovery definitively fails (in _get_name_version_from_pyproject, covering the missing-file, read-failure, and no-metadata branches), naming that no PKG-INFO was found, the possible 3.12+ setuptools/distutils cause, and that pyproject.toml has no static [project] name/version. The success path stays clean — the recovery case still logs only at info/debug. Added tests asserting the warning fires on terminal failure and stays silent on success; all 112 pass.
| # `setup.py egg_info` to read, which fails outright in Python | ||
| # 3.12+ build environments where setuptools is not installed. | ||
| # Fall back to the PEP 621 [project] metadata in pyproject.toml. | ||
| name, version = self._get_name_version_from_pyproject(package_dir) |
There was a problem hiding this comment.
[BUG] The version recovered from pyproject.toml is the raw author-written string, but every other producer of a version in this module supplies the PEP 440 canonical form. That asymmetry can turn a clear failure into a confusing one.
Package.__eq__/__hash__ compare on identifier ("{normalized_name}=={version}"), and the wheel side of that comparison comes from the wheel filename:
# Package._calculate_name_and_version
if self.dist_type == "wheel":
name, version = self.filename.split("-")[:2] # canonical PEP 440 versionDependencyBuilder._download_dependencies then relies on exact string equality:
missing_wheels = sdists - compatible_wheels
...
missing_wheels = deps - compatible_wheels
return compatible_wheels, missing_wheelsand build_site_packages raises MissingDependencyError for anything left in that set.
Concretely, for a git+https requirement whose pyproject.toml declares version = "2024.01.15" (or "1.0.0-rc1", "v1.2.3"), the build backend emits foo-2024.1.15-py3-none-any.whl. The sdist Package is foo==2024.01.15, the wheel Package is foo==2024.1.15, so the sdist never leaves missing_wheels: it gets rebuilt on both _build_sdists passes and the build ends in MissingDependencyError even though the wheel built fine. That is strictly harder to diagnose than the UnsupportedPackageError this PR replaces. This path is unreachable today because PKG-INFO Version is already normalized by the build backend, so it is introduced by the new fallback.
Normalizing the recovered version before returning it keeps the identity comparison consistent — e.g. via packaging.version.Version if you're willing to add packaging to requirements/python_pip.txt. If you'd rather not take the dependency, treating a non-canonical version as unrecoverable (return (None, None) so the existing UnsupportedPackageError path is used) at least preserves the actionable error message instead of surfacing a spurious missing dependency.
1 participant
Issue #, if available:
#675
Description of changes
sam buildfails withUnsupportedPackageError: Unable to retrieve name/version for packagewhenrequirements.txtcontains agit+httpsdependency that is a PEP 517-only project (nosetup.py, no pre-generatedPKG-INFO). This happens on Python 3.12+ build environments wheresetuptoolsis not installed, so thesetup.py egg_infometadata probe fails and there is nothing to fall back to — even thoughpip downloaditself resolved and built the package metadata successfully.When the existing metadata retrieval raises
UnsupportedPackageError,SDistMetadataFetchernow falls back to reading the static PEP 621[project]name/versionfrom the sdist'spyproject.tomlbefore giving up. Parsing uses stdlibtomllibwhere available (Python 3.11+) with a minimal line-based parse of the[project]section as a fallback so this keeps working on Python 3.10. Dynamic versions or missing metadata still raiseUnsupportedPackageErroras before.Description of how you validated changes
pip downloadsaves for agit+httpsrequirement) raisedUnsupportedPackageErrorbefore the fix and now resolves to the correctname/version.tomllibpath).black --checkclean;ruffshows only the pre-existing baseline findings (verified identical count on unmodified code).Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.