Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stop swallowing warnings from Pex by default #20480

Merged
merged 7 commits into from
Feb 15, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
7 changes: 5 additions & 2 deletions src/python/pants/backend/python/goals/lockfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
from pants.backend.python.util_rules.lockfile_diff import _generate_python_lockfile_diff
from pants.backend.python.util_rules.lockfile_metadata import PythonLockfileMetadata
from pants.backend.python.util_rules.pex_cli import PexCliProcess
from pants.backend.python.util_rules.pex_cli import PexCliProcess, maybe_log_pex_stderr
from pants.backend.python.util_rules.pex_environment import PexSubsystem
from pants.backend.python.util_rules.pex_requirements import (
PexRequirements,
ResolvePexConfig,
Expand Down Expand Up @@ -103,6 +104,7 @@ async def generate_lockfile(
req: GeneratePythonLockfile,
generate_lockfiles_subsystem: GenerateLockfilesSubsystem,
python_setup: PythonSetup,
pex_subsystem: PexSubsystem,
) -> GenerateLockfileResult:
pip_args_setup = await _setup_pip_args_and_constraints_file(req.resolve_name)

Expand All @@ -113,7 +115,6 @@ async def generate_lockfile(
subcommand=("lock", "create"),
extra_args=(
"--output=lock.json",
"--no-emit-warnings",
# See https://github.com/pantsbuild/pants/issues/12458. For now, we always
# generate universal locks because they have the best compatibility. We may
# want to let users change this, as `style=strict` is safer.
Expand Down Expand Up @@ -158,6 +159,8 @@ async def generate_lockfile(
),
)

maybe_log_pex_stderr(result.stderr, pex_subsystem.verbosity)

initial_lockfile_digest_contents = await Get(DigestContents, Digest, result.output_digest)
metadata = PythonLockfileMetadata.new(
valid_for_interpreter_constraints=req.interpreter_constraints,
Expand Down
35 changes: 35 additions & 0 deletions src/python/pants/backend/python/target_types.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

from pants.backend.python.macros.python_artifact import PythonArtifact
from pants.backend.python.subsystems.setup import PythonSetup
from pants.base.deprecated import warn_or_error
from pants.core.goals.generate_lockfiles import UnrecognizedResolveNamesError
from pants.core.goals.package import OutputPathField
from pants.core.goals.run import RestartableField
Expand Down Expand Up @@ -381,6 +382,24 @@ class PexArgsField(StringSequenceField):
)


class PexCheckField(StringField):
alias = "check"
valid_choices = ("none", "warn", "error")
expected_type = str
default = "error"
help = help_text(
"""
Check that the built PEX is valid. Currently this only
applies to `--layout zipapp` where the PEX zip is
tested for importability of its `__main__` module by
the Python zipimport module. This check will fail for
PEX zips that use ZIP64 extensions since the Python
zipimport zipimporter only works with 32 bit zips. The
check no-ops for all other layouts.
"""
)


class PexEnvField(DictStringToStringField):
alias = "env"
help = help_text(
Expand Down Expand Up @@ -545,6 +564,19 @@ class PexEmitWarningsField(TriBoolField):
def value_or_global_default(self, pex_binary_defaults: PexBinaryDefaults) -> bool:
if self.value is None:
return pex_binary_defaults.emit_warnings
warn_or_error(
"2.21.0.dev0",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a relatively small amount of code, so maybe we could extend the deprecation for longer, to be nicer to users/make it easier to upgrade (e.g. if someone is using 2.19.0, and then testing major features/fixes in a hypothetical 2.20.0rc3 and 2.21.0.dev4, it's nice if they're not completely blocked until they make changes for "minor" stuff like shuffling fields around, which removing this in 2.21.0.dev0 would force.)

Suggested change
"2.21.0.dev0",
"2.23.0.dev0",

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the fix-instead-of-deprecate option shot per other review feedback.

softwrap(
"""
`emit_warnings` no longer set per target.
Use `emit_warnings` on the [pex] subsytem to control warnings for
all pex invocations
"""
),
"use [pex] subsystem instead",
start_version="2.20.0.dev0",
)

return self.value


Expand Down Expand Up @@ -697,6 +729,7 @@ class PexVenvHermeticScripts(BoolField):
InterpreterConstraintsField,
PythonResolveField,
PexBinaryDependenciesField,
PexCheckField,
PexPlatformsField,
PexCompletePlatformsField,
PexResolveLocalPlatformsField,
Expand Down Expand Up @@ -837,6 +870,8 @@ class PexBinaryDefaults(Subsystem):
`pex_binary` targets
"""
),
removal_version="2.21.0.dev0",
removal_hint="Use the [pex] subsystem to control warnings for all Pex invocations",
advanced=True,
)
resolve_local_platforms = BoolOption(
Expand Down
13 changes: 3 additions & 10 deletions src/python/pants/backend/python/util_rules/pex.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
)
from pants.backend.python.util_rules import pex_cli, pex_requirements
from pants.backend.python.util_rules.interpreter_constraints import InterpreterConstraints
from pants.backend.python.util_rules.pex_cli import PexCliProcess, PexPEX
from pants.backend.python.util_rules.pex_cli import PexCliProcess, PexPEX, maybe_log_pex_stderr
from pants.backend.python.util_rules.pex_environment import (
CompletePexEnvironment,
PexEnvironment,
Expand Down Expand Up @@ -400,10 +400,7 @@ async def find_interpreter(
)
path, fingerprint = result.stdout.decode().strip().splitlines()

if pex_subsystem.verbosity > 0:
log_output = result.stderr.decode()
if log_output:
logger.info("%s", log_output)
maybe_log_pex_stderr(result.stderr, pex_subsystem.verbosity)

return PythonExecutable(path=path, fingerprint=fingerprint)

Expand Down Expand Up @@ -675,7 +672,6 @@ async def build_pex(
argv = [
"--output-file",
request.output_filename,
"--no-emit-warnings",
*request.additional_args,
]

Expand Down Expand Up @@ -746,10 +742,7 @@ async def build_pex(
),
)

if pex_subsystem.verbosity > 0:
log_output = result.stderr.decode()
if log_output:
logger.info("%s", log_output)
maybe_log_pex_stderr(result.stderr, pex_subsystem.verbosity)

digest = (
await Get(
Expand Down
15 changes: 15 additions & 0 deletions src/python/pants/backend/python/util_rules/pex_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from __future__ import annotations

import dataclasses
import logging
import os.path
from dataclasses import dataclass
from typing import Iterable, List, Mapping, Optional, Tuple
Expand All @@ -29,6 +30,8 @@
from pants.util.logging import LogLevel
from pants.util.meta import classproperty

logger = logging.getLogger(__name__)


class PexCli(TemplatedExternalTool):
options_scope = "pex-cli"
Expand Down Expand Up @@ -156,6 +159,8 @@ async def setup_pex_cli_process(

verbosity_args = [f"-{'v' * pex_subsystem.verbosity}"] if pex_subsystem.verbosity > 0 else []

warnings_args = [] if pex_subsystem.emit_warnings else ["--no-emit-warnings"]

# NB: We should always pass `--python-path`, as that tells Pex where to look for interpreters
# when `--python` isn't an absolute path.
resolve_args = [
Expand All @@ -171,6 +176,7 @@ async def setup_pex_cli_process(
*request.subcommand,
*global_args,
*verbosity_args,
*warnings_args,
*pip_version_args,
*resolve_args,
# NB: This comes at the end because it may use `--` passthrough args, # which must come at
Expand Down Expand Up @@ -202,6 +208,15 @@ async def setup_pex_cli_process(
)


def maybe_log_pex_stderr(stderr: bytes, pex_verbosity: int) -> None:
"""Forward Pex's stderr to a Pants logger if conditions are met."""
log_output = stderr.decode()
if log_output and pex_verbosity > 0:
logger.info("%s", log_output)
elif log_output and "PEXWarning:" in log_output:
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about potentially being even more lax here: I would think showing more warnings is better than missing some, e.g. just check for warning (case insensitively)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm worried about making a wishy washy guess that someday might cause someone to have to think about unicode case folding in a whacky corner case. I'd rather either have the PEXWarning check, or stderr unconditionally.

logger.warning("%s", log_output)


def rules():
return [
*collect_rules(),
Expand Down
8 changes: 8 additions & 0 deletions src/python/pants/backend/python/util_rules/pex_environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,14 @@ def iter_path_entries():
),
advanced=True,
)
emit_warnings = BoolOption(
default=True,
help=softwrap(
"""
If warnings from Pex should be logged by Pants to the console.
"""
),
)

@property
def verbosity(self) -> int:
Expand Down
Loading