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

[feature] add verbose mode #8

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

c7h
Copy link

@c7h c7h commented May 26, 2020

I had the problem that for a batch task, I only needed the symbol printed to stdout. Therefore, I made the meta information optional.

@marin-m
Copy link
Owner

marin-m commented May 26, 2020

Hello,

Thanks for the PR, having this optional is probably good.

  • Having an amount of debugging visible by default is likely useful to understand better the output for the tools, and to debug it if needed, for most users: is it ok if I turn your -v, --verbose flag into a -s, --silent flag?
  • I see that you have removed the architecture guess duration counter from the code, and moved the print call out of the original function, likely to be able to more easily reference the self.verbose attribute by keeping it as an attribute of one class. I think I should maybe reintroduce the guess time counter.
  • Just out of curiosity, I would like to know how having a --verbose/--silent flag meet your need compared to having the debugging output simply going through stderr?

Thanks again for the time taken for this pull request,

Regards,

@c7h
Copy link
Author

c7h commented May 26, 2020

Hi @marin-m
Thanks for the quick response.

  • Yes, changing the flag to --silent is a good idea to avoid a breaking change. I will update my PR.
  • I can also add the duration counter back in. It just wasn't useful for me but I agree that It might be for others.
  • We could also just write the debug output to stderr. I would make this optional to avoid a breaking change here. How about in case of --silent, we print the debug output to stderr instead of stdout? This way, we could keep the stdout clean, we would be backwards compatible but still show the debug messages to the user.

Looking forward to hear your opinion,
Cheers,
Chris

@marin-m
Copy link
Owner

marin-m commented May 26, 2020

Hello,

We could also just write the debug output to stderr. I would make this optional to avoid a breaking change here. How about in case of --silent, we print the debug output to stderr instead of stdout?

It was just to know. I think that it is fine to keep everything going to stdout as it is currently, as:

  • The --silent flag will then actually silence the debugging output, which is a more intuitive behavior than redirecting streams.
  • Providing one way to separate both outputs (the flag) is still correct, and fulfill the needs in a simple manner.
  • Backwards compatibility is not critical I think as the tool is still recent, but if we can keep it behaving like this it is still good. Especially, both executables output the same debugging output, and the debugging output is also an information output which is the primary output that the user may want to log when running the vmlinux-to-elf executable.

Regards,

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.

2 participants