-
Notifications
You must be signed in to change notification settings - Fork 989
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
Add json output to info command (both console and file) #4359
Add json output to info command (both console and file) #4359
Conversation
@@ -475,8 +475,7 @@ def info(self, *args): | |||
"specify both install-folder and any setting/option " | |||
"it will raise an error.") | |||
parser.add_argument("-j", "--json", nargs='?', const="1", type=str, | |||
help='Only with --build-order option, return the information in a json.' | |||
' e.g --json=/path/to/filename.json or --json to output the json') | |||
help='Path to a json file where the information will be written') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've intentionally removed the mention to --json
(without value) will output to console, I think it is something we don't want to promote. Shall I revert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not promote? isn't such text interface used as a base for unix command-line pipelining, e.g. conan --json | some_tool | some_other_tool
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cannot make this commitment right now, there are some commands that are outputting a lot of information to the console and then generating the json data (like the info
one with the profiles information), so you cannot pipe the output to the next tool.
It would be a nice-to-have, it will require to silent all the output and then just print the JSON data. Something to do, not so hard, but not yet.
More issues related to output: #4225 (what we are talking here), and two more issues also worth reading #4215, #4067
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we can just remove this despite the fact that I don't like it as it is either. We will need to discuss it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm talking about removing it just from the --help
output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, it is fine for me
ref = str(conanfile) | ||
|
||
item_data["reference"] = str(ref) | ||
item_data["is_ref"] = isinstance(ref, ConanFileReference) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need this field in the JSON to perform some logic in the console output: will output Remote: None
if it is a ConanFileReference
, but it won't if it isn't... same with Binary remote:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... if we do not consider breaking to remove this Remote: None
I will be very pleased to get rid of them
@@ -475,8 +475,7 @@ def info(self, *args): | |||
"specify both install-folder and any setting/option " | |||
"it will raise an error.") | |||
parser.add_argument("-j", "--json", nargs='?', const="1", type=str, | |||
help='Only with --build-order option, return the information in a json.' | |||
' e.g --json=/path/to/filename.json or --json to output the json') | |||
help='Path to a json file where the information will be written') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we can just remove this despite the fact that I don't like it as it is either. We will need to discuss it
item_data["export_folder"] = self.cache.export(ref) | ||
item_data["source_folder"] = self.cache.source(ref, conanfile.short_paths) | ||
if isinstance(self.cache, SimplePaths): | ||
# @todo: check if this is correct or if it must always be package_id() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the package_id
already in the main for
iteration. Why to do all this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I can remove one line, the package_id
variable is already available in the for
; but the block related to the build_folder
and the @todo
section come from #1032, it should require a deeper rationale.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The two calls to conanfile.info.package_id()
yes. I understand the rest is necessary.
else: | ||
if args.json: | ||
json_arg = True if args.json == "1" else args.json | ||
self._outputer.json_info(deps_graph, json_arg, get_cwd(), show_paths=args.paths) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could output always the paths when json output and forbid the args.paths
and args.json
together. It simplifies a bit everything and I would expect a json to be always complete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For packages installed as editable access to paths raises. There are two options:
- keep the argument, if the user asks for paths, it will raise.
- always True and try/except the paths section, paths won't be returned and the user won't know why.
data = [str(n) for n in nodes_to_build] | ||
self._handle_json_output(data, json_output, cwd) | ||
|
||
def _grab_info_data(self, deps_graph, grab_paths): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this "conversion function" doesn't belong to this module. A to_json
in the graph? Not very fan either...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is created and called from outside the conan_api, the graph shouldn't have left the api. I'm sure that this json should be the output of the conan_api::info
function, but this would be a refactor (I vote for it) and not the objective of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Open the engineering issue, please.
Changelog: Feature: Add JSON output to 'info' command
Docs: conan-io/docs#1050
develop
branch, documenting this one.It digests the information of the graph and then outputs it using JSON format (all the fields) or through the console (some logic still there)