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

Output of 'pdm export' gets line wrapped even when the output isn't a terminal #2390

Closed
1 task done
datalogics-kam opened this issue Nov 9, 2023 · 12 comments · Fixed by #2397
Closed
1 task done
Labels
🐛 bug Something isn't working 💝 good first issue Good for newcomers

Comments

@datalogics-kam
Copy link

datalogics-kam commented Nov 9, 2023

  • I have searched the issue tracker and believe that this is not a duplicate.

Make sure you run commands with -v flag before pasting the output.

Steps to reproduce

In the actual case, we're using subprocess.run(...,stdout=subprocess.PIPE) to consume the output from pdm export. We make a hash and then decide if we want to write some files.

I've been trying to make a small test case, but I haven't figured out what is causing the wrapping behavior. The actual run of PDM is buried in a series of nested subprocesses, and I haven't found anything that would trigger Rich to wrap, like the environment variable COLUMNS being set, or TERM being set to a dumb terminal.

Actual behavior

It seems to work fine most of the time locally, but when running from inside pytest the file gets corrupted: A long line with --extra-index-url with our AWS CodeArtifact URL in it gets line wrapped, and then pip can't use the file.

--extra-index-url 
https://aws:${CIT_PYPI_PASSWORD}@xxxxxxxxxx-xxxxxxxxxxxx.d.codeartifact.us-east-
2.amazonaws.com/pypi/cit-pypi/simple/

Expected behavior

I would expect that in no case would the output of pdm export be formatted in any way. Even if the output is a bit wide for the terminal, it should be consumable by, for instance, pip.

The question I have is whether the export output should be sent through Rich.

Environment Information

# Paste the output of `pdm info && pdm info --env` below:
❯ pdm info && pdm info --env
PDM version:
  2.10.1
Python Interpreter:
  /Users/kam/Desktop/narrow-lines/.venv/bin/python (3.11)
Project Root:
  /Users/kam/Desktop/narrow-lines
Local Packages:

{
  "implementation_name": "cpython",
  "implementation_version": "3.11.2",
  "os_name": "posix",
  "platform_machine": "x86_64",
  "platform_release": "22.6.0",
  "platform_system": "Darwin",
  "platform_version": "Darwin Kernel Version 22.6.0: Fri Sep 15 13:39:52 PDT 2023; root:xnu-8796.141.3.700.8~1/RELEASE_X86_64",
  "python_full_version": "3.11.2",
  "platform_python_implementation": "CPython",
  "python_version": "3.11",
  "sys_platform": "darwin"
}
@datalogics-kam datalogics-kam added the 🐛 bug Something isn't working label Nov 9, 2023
@frostming
Copy link
Collaborator

We support output to file flag --output, why not use that?

@pawamoy
Copy link
Contributor

pawamoy commented Nov 10, 2023

If using --output doesn't wrap the output, that's great, but it requires more gymnastics to use it (see below), and I guess @datalogics-kam's point is that the output should never be wrapped, whether --output is used or not.

With subprocess, I suppose using --output requires creating a temporary file, then reading again from it once the command has finished.

import os
import subprocess
from tempfile import TemporaryDirectory

with TemporaryDirectory() as tmpdir:
    tmpfile = os.path.join(tmpdir, "requirements.txt")
    subprocess.run(f"pdm export -o {tmpfile}".split())
    with open(tmpfile) as file:
        requirements = file.read()

Using requirements = subprocess.check_output("pdm export") is arguably much easier 🙂

I'm actually doing the same thing as @datalogics-kam in my project tasks to check my dependencies:

@duty
def check_dependencies(ctx: Context) -> None:
    requirements = ctx.run(
        ["pdm", "export", "-f", "requirements", "--without-hashes"],
        title="Exporting dependencies as requirements",
        allow_overrides=False,
    )

    ctx.run(
        safety.check(requirements),
        title="Checking dependencies",
        command="pdm export -f requirements --without-hashes | safety check --stdin",
    )

Apparently I'm just lucky that none of them use an extra index URL 🤔 I'll verify that to confirm.

@pawamoy
Copy link
Contributor

pawamoy commented Nov 10, 2023

It seems to work fine most of the time locally, but when running from inside pytest the file gets corrupted

Is it possible that pytest is the culprit here? Without seeing your code and how you test it, it's difficult to guess 🤔

@datalogics-kam
Copy link
Author

Is it possible that pytest is the culprit here? Without seeing your code and how you test it, it's difficult to guess 🤔

It breaks in a situation where there are a lot of layers...coverage calls pytest calls (AWS) cdk synth which is using our custom PythonLayerVersion which wants the requirements from a certain group.

I've tried probing to see what's going on, but my best guess is that something along the way is using a pty or setting environment variables that makes Rich think the output is a terminal that's 80 columns wide.

It's also not real consistent. I tried making a small pyproject.toml that demonstrates the problem, and couldn't get it to consistently break in that case.

For now I did in fact use --output with a temporary file, but conceptually I thought of pdm export as a sophisticated version of pip freeze and just used the stdout without exploring other possibilities.

@frostming
Copy link
Collaborator

I'm not opposed to changing to a regular print, can someone send a PR?

@frostming frostming added the 💝 good first issue Good for newcomers label Nov 12, 2023
@pawamoy
Copy link
Contributor

pawamoy commented Nov 12, 2023

Erm, I'm so sorry @frostming, I pushed directly to the main branch 😱
I'm not used to having push access on repositories I only contribute to, and GitHub's web interface offered me this choice by default. I clicked too fast. Sorry 🙇 Don't hesitate to git reset, and I'll open a proper PR. Also I wouldn't mind that you remove my push access to the main branch 😅

@pawamoy
Copy link
Contributor

pawamoy commented Nov 12, 2023

By the way I can probably do the git reset + force push, but I don't want to make any more mistake, so I'll wait for your input 🙂

@frostming
Copy link
Collaborator

Don't worry about it. I think we should protect the main branch, but sometimes I like to push commits to it, which may not be a good habit.😟

@pawamoy
Copy link
Contributor

pawamoy commented Nov 12, 2023

I think it's possible to remove push access to the main branch for specific collaborators only, while keeping it for yourself.

@frostming
Copy link
Collaborator

@pawamoy done, now you continue with your PR.

@pawamoy
Copy link
Contributor

pawamoy commented Nov 13, 2023

Thanks!

I'm not sure why but I'm getting 72 Mypy errors when pre-commit runs, preventing me from committing, so I'll just disable the Git hook for now.

@datalogics-kam
Copy link
Author

Thanks, @pawamoy, for the fix!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working 💝 good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants