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

proposing new --format=<format> --out-file=<file.ext> feature #17507

Draft
wants to merge 10 commits into
base: develop2
Choose a base branch
from

Conversation

memsharded
Copy link
Member

@memsharded memsharded commented Dec 19, 2024

Changelog: Feature: Allow defining --out-file=file.ext instead of --format=ext > file.ext to write to files directly and avoid issues with redirects.
Docs: https://github.com/conan-io/docs/pull/XXXX

This would solve issues with stdout/stderr redirects and potential pollution, while not using new arguments, not breaking existing code and allow easy introduction to existing formatters.

Close #17462

@czoido
Copy link
Contributor

czoido commented Dec 19, 2024

Related to #17462

Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Question, otherwise looks good, but I think checking with the team in as look-into is a good idea first, good call

"""
Output to be used by formatters to dump information to stdout
"""
if filename is not None:
ConanOutput().info(f"Formatted output saved to '{filename}'")
save(filename, data)
Copy link
Member

Choose a reason for hiding this comment

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

There are a few commands whose formatter calls cli_out_write multiple times in a row.

Should we convert those to only call it once, or should this new functionality support appending?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Not sure to be honest, I tend to think to re-write the formatter to include only 1 call, but there are some case that use colors and that would be a bit less elegant, but still doable

@czoido
Copy link
Contributor

czoido commented Dec 19, 2024

I'm not sure I prefer this over having a new --output-file argument to specify the file... I was thinking... what about something at the midpoint like: --format="json:myfile.json" ?

@memsharded
Copy link
Member Author

I'm not sure I prefer this over having a new --output-file argument to specify the file... I was thinking... what about something at the midpoint like: --format="json:myfile.json" ?

Yes, I like that midpoint. I thought about the extra argument, but that further pollutes the CLI, like --format=json --output-file=graph.json is like "Hey, I am already telling you in the filename that I want a json file, don't be that dumb".

But if you think the --output-file is better, we can do it too, I am not strongly opposed either.

@perseoGI
Copy link
Contributor

I agree with having another CLI option, it might be confusing that the fact of defining an output format could also lead to defining the output file.

In conanlint we have made something similar:
image

Also, it may cause issues with future formats which could contain a dot delimiter.

All in all, the feature is a good one! Some console redirections are not always consistent.
(In the linter I add this option to allow progress bar + redirection to file)

@czoido
Copy link
Contributor

czoido commented Dec 19, 2024

I'm not sure I prefer this over having a new --output-file argument to specify the file... I was thinking... what about something at the midpoint like: --format="json:myfile.json" ?

Yes, I like that midpoint. I thought about the extra argument, but that further pollutes the CLI, like --format=json --output-file=graph.json is like "Hey, I am already telling you in the filename that I want a json file, don't be that dumb".

But if you think the --output-file is better, we can do it too, I am not strongly opposed either.

The main thing for me is to maintain the concept of a formatter. A --format=file.json argument doesn’t feel like a formatter anymore. The original idea was that the formatter should define how the output is formatted (HTML, JSON, etc.), not where it’s written. So, for me, any idea that maintains in some way the meaning of the formatter is ok.

@czoido czoido marked this pull request as draft December 19, 2024 17:53
Copy link
Member Author

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

Looking good

@@ -192,6 +192,9 @@ def graph_info(conan_api, parser, subparser, *args):
help="Deployer output folder, base build folder by default if not set")
subparser.add_argument("--build-require", action='store_true', default=False,
help='Whether the provided reference is a build-require')
subparser.add_argument("--out-file", action=OnceArgument,
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be injected automatically and conditionally to the existence of formatters together with the definition of the --format argument. There cannot be a --out-file without a --format definition.

Copy link
Contributor

@czoido czoido Dec 20, 2024

Choose a reason for hiding this comment

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

Finally, my proposal is to add the --out-file always, to enable users do for example
conan config home --out-file=file.ext
and it uses the text formatter that is the one by default. Because in the end, there's always a formatter there that is the text one even if it does not show in the --help, but let's discuss about this with the team.

EDIT: Looks like this works in the test suite but not in my CLI, I'm leaving the PR in draft until I understand why.

@czoido czoido changed the title proposing new --format=file.ext feature proposing new --format=<format> --out-file=<file.ext> feature Dec 20, 2024
@czoido czoido marked this pull request as ready for review December 20, 2024 06:45
@czoido czoido marked this pull request as draft December 20, 2024 07:03
@czoido
Copy link
Contributor

czoido commented Dec 20, 2024

I'm leaving this as draft, apparently these lines:

conan/conan/api/output.py

Lines 298 to 300 in 003e2cf

colorama.deinit()
sys.stdout.write(data)
colorama.reinit()

are making that this approach redirecting the stdout do not work on a real console but they do work on testing because we capture the streams. I'll investigate a solution that also prevent #17245 to not happen any more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Warnings from Python libraries pollute json output
4 participants