Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
191 changes: 186 additions & 5 deletions aws_lambda_builders/workflows/python_pip/packager.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
import re
import subprocess
from email.parser import FeedParser
from typing import List, Tuple
from typing import List, Optional, Tuple

try:
import tomllib
except ImportError: # Python 3.10 does not have tomllib in the standard library
tomllib = None

from aws_lambda_builders.architecture import ARM64, X86_64
from aws_lambda_builders.utils import extract_tarfile
Expand All @@ -18,6 +23,40 @@
LOG = logging.getLogger(__name__)


# Matches a quoted scalar value with an optional trailing comment, e.g.
# "1.2.3" -> group(2) == 1.2.3
# 'foo' # comment -> group(2) == foo
QUOTED_VALUE = re.compile(r"""^(["'])(.*?)\1\s*(?:#.*)?$""")


def _canonicalize_version(version):
"""
Return the PEP 440 canonical form of an author-written version string.

Every other version producer in this module (PKG-INFO, wheel filenames)
supplies the canonical form, and ``Package`` identity comparison is an
exact string match -- so a non-canonical ``pyproject.toml`` version would
never reconcile with the wheel built from it. Returns None when the
version is not valid PEP 440, in which case the package is treated as
unrecoverable.

The ``packaging`` import is local and degradable: this is a rare fallback
path, and importing it at module scope would make it a hard import-time
requirement for the entire python_pip workflow (PLC0415 is already in the
ruff ignore list, so a function-local import is idiomatic here).
"""
try:
from packaging.version import InvalidVersion, Version
except ImportError:
LOG.debug("packaging is unavailable; using the pyproject.toml version as written")
return version
try:
return str(Version(version))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[GENERAL] Importing packaging at module scope makes a rare fallback path a hard import-time requirement for the entire python_pip workflow.

requirements/base.txt is empty today, and requirements/python_pip.txt listed only setuptools/wheel — which are needed by the setup.py egg_info subprocess, not imported by this module. from packaging.version import InvalidVersion, Version is therefore the first import-time third-party dependency of the library itself. If packaging is missing or incompatible in any environment that embeds this library (vendored copies, frozen/PyInstaller builds where hidden imports must be declared explicitly), import ...python_pip.packager fails outright and every Python build breaks — not just the pyproject.toml metadata recovery that actually needs it.

Since PLC0415 (import-outside-top-level) is already in the ruff ignore list, a locally scoped, degradable import is idiomatic for this codebase:

def canonicalizeversion(version):
   try:
       from packaging.version import InvalidVersion, Version
   except ImportError:
       LOG.debug("packaging is unavailable; using the pyproject.toml version as written")
       return version
   try:
       return str(Version(version))
   except InvalidVersion:
       LOG.debug("pyproject.toml version %r is not a valid PEP 440 version", version)
       return None

This keeps the canonicalization behavior when packaging is installed (the declared case) while confining the failure mode to the fallback path instead of the whole workflow.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — the import moved inside _canonicalize_version, so the module no longer hard-depends on packaging at import time. If packaging is missing, it logs at debug and returns the version as-is.

except InvalidVersion:
LOG.debug("pyproject.toml version %r is not a valid PEP 440 version", version)
return None


# TODO update the wording here
MISSING_DEPENDENCIES_TEMPLATE = r"""
Could not install dependencies:
Expand All @@ -31,6 +70,71 @@ class PackagerError(Exception):
pass


def _finalize_name_version(name, version) -> Tuple[Optional[str], Optional[str]]:
"""
Validate a parsed (name, version) pair and normalize the version to its
PEP 440 canonical form. Returns (None, None) when the pair is unusable
(missing values, non-string values, or a version that is not valid
PEP 440).
"""
if not isinstance(name, str) or not isinstance(version, str):
return None, None
canonical_version = _canonicalize_version(version)
if canonical_version is None:
return None, None
return name, canonical_version


def _parse_pyproject_name_version(contents: str) -> Tuple[Optional[str], Optional[str]]:
"""
Reads the PEP 621 ``[project]`` name and version from pyproject.toml contents.

Returns (None, None) when no usable static name/version can be determined
(missing table, dynamic version, unparsable file, or a version that is not
valid PEP 440). The returned version is normalized to its PEP 440
canonical form so it matches the wheel filename produced by the build
backend. Uses stdlib
``tomllib`` when available (Python 3.11+) and a minimal line-based parse
of the ``[project]`` section otherwise, so this keeps working on
Python 3.10. The line-based parse only accepts properly quoted scalars and
tolerates trailing comments.
"""
if tomllib is not None:
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):
# `project = "something"` is valid TOML but not a table; do not
# assume shape on third-party input.
LOG.debug("pyproject.toml [project] is not a table; cannot read name/version")
return None, None
return _finalize_name_version(project.get("name"), project.get("version"))
name, version = None, None
in_project_section = False
for line in contents.splitlines():
stripped = line.strip()
if stripped.startswith("["):
# tolerate trailing comments on the header, e.g. "[project] # main"
in_project_section = stripped.split("#")[0].strip() == "[project]"
continue
if not in_project_section or stripped.startswith("#") or "=" not in stripped:
continue
key, _, value = stripped.partition("=")
match = QUOTED_VALUE.match(value.strip())
if not match:
# Skip anything that is not a quoted scalar (e.g. dynamic = ["version"])
continue
value = match.group(2)
if key.strip() == "name" and not name:
name = value or None
elif key.strip() == "version" and not version:
version = value or None
return _finalize_name_version(name, version)


class InvalidSourceDistributionNameError(PackagerError):
pass

Expand Down Expand Up @@ -727,7 +831,11 @@ def _get_pkg_info_filepath(self, package_dir):
LOG.debug("Error while searching for existing .egg-info directories: %s", e)

if not self._osutils.file_exists(pkg_info_path):
LOG.warning(
# This used to be a warning, but the caller may now recover via
# pyproject.toml metadata, in which case the build succeeds and
# a warning would be misleading. Keep it at debug; the caller
# logs an explicit message when recovery succeeds.
LOG.debug(
"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.",
Expand Down Expand Up @@ -755,6 +863,66 @@ def _get_fallback_pkg_info_filepath(self, package_dir: str) -> str:

return pkg_info_path

def _get_name_version_from_pyproject(self, package_dir: str) -> Tuple[str, str]:
"""
Extracts the name and version from the PEP 621 [project] table of a
pyproject.toml file.

This is a last resort for sdists that carry no setup.py or PKG-INFO
metadata (e.g. PEP 517-only projects downloaded from git+https URLs),
where `setup.py egg_info` cannot produce any metadata.

Parameters
----------
package_dir: str
The path of the unpacked sdist directory

Returns
-------
Tuple[str, str]
A tuple containing the name and version

Raises
------
UnsupportedPackageError
If no usable static name/version can be read from pyproject.toml
"""
pyproject_path = self._osutils.joinpath(package_dir, "pyproject.toml")
if not self._osutils.file_exists(pyproject_path):
self._warn_unrecoverable_metadata(package_dir)
raise UnsupportedPackageError(self._osutils.basename(package_dir))
try:
# utf-8-sig also tolerates a BOM so BOM-prefixed files still parse.
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)
self._warn_unrecoverable_metadata(package_dir)
raise UnsupportedPackageError(self._osutils.basename(package_dir)) from ex
name, version = _parse_pyproject_name_version(contents)
if not name or not version:
self._warn_unrecoverable_metadata(package_dir)
raise UnsupportedPackageError(self._osutils.basename(package_dir))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOG.debug("Using name/version from pyproject.toml [project] table: %s==%s", name, version)
return name, version

@staticmethod
def _warn_unrecoverable_metadata(package_dir: str) -> None:
"""
Emits the user-visible diagnostic when no metadata source remains.

The PKG-INFO probe now logs at debug because the pyproject.toml
fallback may recover; this warning is emitted only at the point where
recovery definitively fails so `sam build` output still names the
likely cause instead of just the opaque UnsupportedPackageError.
"""
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,
)

def _unpack_sdist_into_dir(self, sdist_path, unpack_dir):
if sdist_path.endswith(".zip"):
self._osutils.extract_zipfile(sdist_path, unpack_dir)
Expand Down Expand Up @@ -820,9 +988,22 @@ def get_package_name_and_version(self, sdist_path: str) -> Tuple[str, str]:
with self._osutils.tempdir() as tempdir:
package_dir = self._unpack_sdist_into_dir(sdist_path, tempdir)

# 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)
try:
# 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:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# PEP 517-only sdists (e.g. downloaded from a git+https
# requirement) may carry no setup.py or PKG-INFO metadata for
# `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)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 version

DependencyBuilder._download_dependencies then relies on exact string equality:

missing_wheels = sdists - compatible_wheels
...
missing_wheels = deps - compatible_wheels
return compatible_wheels, missing_wheels

and 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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — the raw author-written string (e.g. 2024.01.15, 1.0.0-rc1) would never match the wheel filename the backend produces. Versions from both parser paths are now normalized to PEP 440 canonical form via packaging.Version, and anything that isn't valid PEP 440 falls back to (None, None) as before. Added packaging to requirements/python_pip.txt (which setup.py already includes in install_requires).

LOG.info(
"Recovered name/version for package in %s from pyproject.toml "
"[project] table; PKG-INFO metadata was unavailable.",
package_dir,
)

# return values if it is not the default values
if not self._is_default_setuptools_values(name, version):
Expand Down
5 changes: 4 additions & 1 deletion requirements/python_pip.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,4 +2,7 @@
# Following packages are required by `python_pip` workflow to run.
# TODO: Consider moving this dependency directly into the `python_pip` workflow module
setuptools
wheel
wheel
# Used to normalize PEP 621 versions to PEP 440 canonical form when reading
# name/version from pyproject.toml
packaging
Loading