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

Jump to bottom

Unsolicited messages output to stdout #6427

Closed
rdaysky opened this issue Sep 6, 2022 · 2 comments · Fixed by #6429
Closed

Unsolicited messages output to stdout #6427

rdaysky opened this issue Sep 6, 2022 · 2 comments · Fixed by #6429
Labels
kind/bug Something isn't working as expected

Comments

@rdaysky
Copy link

rdaysky commented Sep 6, 2022 •
edited
Loading

As requested by @neersighted in #1611, opening a new issue.

  1. Install Poetry 1.2.0
  2. poetry config virtualenvs.create false
  3. Try to run something using poetry run. Observe that a message “Skipping virtualenv creation, as specified in config file.” is output to stdout:
$ poetry run date 2>/dev/null
Skipping virtualenv creation, as specified in config file.
Tue  6 Sep 16:07:21 UTC 2022

Or try poetry export:

$ poetry export >export.stdout 2>export.stderr
$ cat export.stdout
Skipping virtualenv creation, as specified in config file.
Skipping virtualenv creation, as specified in config file.
Updating dependencies
Resolving dependencies...

Writing lock file
useless-py==1.0.3 ; python_version >= "3.10" and python_version < "4.0" \
    --hash=sha256:27117ffe3044ffeee36af4572c224e445e04878668c45b133cfe606998c73b2a
$ cat export.stderr
The lock file does not exist. Locking.

The way -q affects this is inconsistent. With run it removes the virtualenv message, with export it removes everything, making the command useless (unless -o is specified). And while it’s a step in the right direction, someone can write a script using, for example, poetry run scripts/make-some-json | jq ... and it will work under the default settings but not for someone who has set up the same codebase with virtualenvs.create=false (for example, in a container). You can’t expect developers to add -q to all poetry invocations in scripts just in case at a later point poetry decides to add something to the output.

Suggested resolution: whenever there’s a “main” output (the machine-readable output of poetry export, or poetry run that could be anything) that could be saved into a file, piped etc., don’t add anything to it unless -v is present, in which case write it to stderr. (When the entirety of the output is generated by poetry itself, e. g. with poetry install, there’s no problem with it printing whatever it likes.)
@rdaysky rdaysky added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Sep 6, 2022
@dimbleby
Copy link
Contributor

dimbleby commented Sep 6, 2022

Some or more of the io.write_line() in /poetry/utils/env.py should be io.write_error_line(), to write to stderr instead of stdout.

MRs welcome, I'm sure.

@neersighted neersighted mentioned this issue Sep 6, 2022
Merged
neersighted added a commit to neersighted/poetry that referenced this issue Sep 6, 2022
@neersighted
fix: write to stderr in utils.env
12cf1e2 
This is a quick fix to avoid polluting stdout unexpectedly when Poetry's
environment management comes into play.

It's apparent from how much the complexity of this file has grown that
this needs to be refactored moderately, as well as each major class
deserving its own source file.

Future work should also include a rethink of how IO objects are passed
around the codebase, how we reason about verbosity at a function level,
and how code is re-used -- one command may wish to output to stdout, but
if that code is reused by another command, the calculus of what is
command output and what is informative (or even needs to be hidden/shown
based on verbosity level) changes.

Work on output would likely have to be fairly comprehensive and
invasive, but things have grown complex enough that a top-down design
pass is likely the best route.

Regardless, this is a simple change today, and low risk. Resolves python-poetry#6427.
neersighted added a commit to neersighted/poetry that referenced this issue Sep 6, 2022
@neersighted
fix: write to stderr in utils.env
5924cfb 
This is a quick fix to avoid polluting stdout unexpectedly when Poetry's
environment management comes into play.

It's apparent from how much the complexity of this file has grown that
this needs to be refactored moderately, as well as each major class
deserving its own source file.

Future work should also include a rethink of how IO objects are passed
around the codebase, how we reason about verbosity at a function level,
and how code is re-used -- one command may wish to output to stdout, but
if that code is reused by another command, the calculus of what is
command output and what is informative (or even needs to be hidden/shown
based on verbosity level) changes.

Work on output would likely have to be fairly comprehensive and
invasive, but things have grown complex enough that a top-down design
pass is likely the best route.

Regardless, this is a simple change today, and low risk. Resolves python-poetry#6427.
neersighted added a commit that referenced this issue Sep 6, 2022
@neersighted
fix: write to stderr in utils.env
47255c4 
This is a quick fix to avoid polluting stdout unexpectedly when Poetry's
environment management comes into play.

It's apparent from how much the complexity of this file has grown that
this needs to be refactored moderately, as well as each major class
deserving its own source file.

Future work should also include a rethink of how IO objects are passed
around the codebase, how we reason about verbosity at a function level,
and how code is re-used -- one command may wish to output to stdout, but
if that code is reused by another command, the calculus of what is
command output and what is informative (or even needs to be hidden/shown
based on verbosity level) changes.

Work on output would likely have to be fairly comprehensive and
invasive, but things have grown complex enough that a top-down design
pass is likely the best route.

Regardless, this is a simple change today, and low risk. Resolves #6427.
@mkniewallner mkniewallner removed the status/triage This issue needs to be triaged label Sep 18, 2022
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
kind/bug Something isn't working as expected
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: write to stderr in utils.env neersighted/poetry
3 participants
@rdaysky @dimbleby @mkniewallner