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.

Sign up for GitHub

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

stop swallowing warnings from Pex by default #20480

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d2d8ce3
stop swalling warnings from Pex by default
cburroughs Feb 1, 2024
3cb4995
deprecate old way
cburroughs Feb 2, 2024
cee1f14
drop spurios cross branch copy
cburroughs Feb 7, 2024
9819b0d
roll back deprecate
cburroughs Feb 7, 2024
b064a69
try to fix old boolean logic
cburroughs Feb 7, 2024
6633bee
warnings be gone!
cburroughs Feb 14, 2024
fe1c470
invert conditional
cburroughs Feb 14, 2024
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
View file
Open in desktop
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
2 changes: 2 additions & 0 deletions src/python/pants/backend/python/goals/package_pex_binary.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ def generate_additional_args(self, pex_binary_defaults: PexBinaryDefaults) -> Tu
args = []
if self.emit_warnings.value_or_global_default(pex_binary_defaults) is False:
args.append("--no-emit-warnings")
elif self.emit_warnings.value_or_global_default(pex_binary_defaults) is True:
args.append("--emit-warnings")
if self.resolve_local_platforms.value_or_global_default(pex_binary_defaults) is True:
args.append("--resolve-local-platforms")
if self.ignore_errors.value is True:
Expand Down
3 changes: 3 additions & 0 deletions src/python/pants/backend/python/goals/package_pex_binary_integration_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ def test_include_sources_avoids_files_targets_warning(
"""\
python_sources(
name='sources',
interpreter_constraints=["CPython==3.10.*"]
)

python_distribution(
Expand All @@ -147,12 +148,14 @@ def test_include_sources_avoids_files_targets_warning(
name='my-dist',
version='1.2.3',
),
interpreter_constraints=["CPython==3.10.*"]
)

pex_binary(
dependencies=[':wheel'],
entry_point="none",
include_sources=False,
interpreter_constraints=["CPython==3.10.*"]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry missed that this test was failing in the pipeline report.

These tighter constarints avoid

ARNING  pants.backend.python.util_rules.pex_cli:pex_cli.py:217 /home/ecsb/.cache/pants/named_caches/pex_root/installed_wheels/f52d67567b65f94aedbde9c10f417df9df1c78a2568cbc7820706a7b3ee6ace2/pex-2.1.159-py2.py3-none-any.whl/pex/bin/pex.py:923: PEXWarning: Could not calculate a targeted shebang for:
cp37-cp37m-manylinux_2_37_x86_64 interpreter at /usr/local/bin/python3.7
cp38-cp38-manylinux_2_37_x86_64 interpreter at /usr/bin/python3.8
cp39-cp39-manylinux_2_37_x86_64 interpreter at /home/ecsb/.cache/pants/named_caches/pex_root/venvs/f2a0eb4c2e2336b7e77df777bff977abf3ca390a/d56abaad3dd7630971f10921587fc3ac5e2cee3c/bin/python3.9
cp39-cp39-manylinux_2_37_x86_64 interpreter at /usr/bin/python3.9

Using shebang: #!/usr/bin/env python3.7
If this is not appropriate, you can specify a custom shebang using the --python-shebang option.
  pex_warnings.warn(

which in term allows the "there is no output" assertion in the test to pass.

)
"""
),
Expand Down
1 change: 1 addition & 0 deletions src/python/pants/backend/python/target_types.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,7 @@ 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

return self.value


Expand Down
13 changes: 3 additions & 10 deletions src/python/pants/backend/python/util_rules/pex.py
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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