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

Add json output to info command (both console and file) #4359

Merged
merged 12 commits into from Jan 29, 2019

Conversation

jgsogo
Copy link
Contributor

@jgsogo jgsogo commented Jan 22, 2019

Changelog: Feature: Add JSON output to 'info' command
Docs: conan-io/docs#1050

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)

@ghost ghost assigned jgsogo Jan 22, 2019
@ghost ghost added the stage: review label Jan 22, 2019
@@ -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')
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'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?

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Member

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

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 talking about removing it just from the --help output.

Copy link
Member

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

conans/client/command.py Outdated Show resolved Hide resolved
ref = str(conanfile)

item_data["reference"] = str(ref)
item_data["is_ref"] = isinstance(ref, ConanFileReference)
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 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:

Copy link
Contributor Author

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

@jgsogo jgsogo requested a review from danimtb January 22, 2019 12:50
@jgsogo jgsogo added this to the 1.12 milestone Jan 22, 2019
conans/client/command.py Show resolved Hide resolved
conans/client/command.py Outdated Show resolved Hide resolved
@@ -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')
Copy link
Member

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()
Copy link
Contributor

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?

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 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.

Copy link
Contributor

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.

conans/client/conan_command_output.py Show resolved Hide resolved
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)
Copy link
Contributor

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.

Copy link
Contributor Author

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):
Copy link
Contributor

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...

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@ghost ghost assigned jgsogo Jan 28, 2019
@ghost ghost assigned jgsogo Jan 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support conan info --json for all outputs, and support stdout
5 participants