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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added logging mechanism to pyGHDL #2120

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

Conversation

eng73
Copy link

@eng73 eng73 commented Jul 3, 2022

Description Please explain the changes you made here.
This PR introduces a basic logger to replace various print statements. The intention is to allow other tools using pyGHDL to be able customise the log level/format/stream.

馃毃 Before submitting your PR, please read contribute in the Docs, and review the following checklist:

When contributing to the GHDL codebase...

  • DO make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your master!
  • DO make sure you are making a pull request against the master branch (left side). Also you should start your branch off our master.
  • DO make sure that GHDL can be successfully built. See Building GHDL.
  • CONSIDER adding a unit test if your PR resolves an issue.
  • CONSIDER modifying the docs, if your contribution is relevant to any of the content.
  • AVOID breaking the continuous integration build.
  • AVOID breaking the testsuite.

When contributing to the docs...

  • DO make sure that the build is successful.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did, what alternatives you considered, etc.

鉂わ笍 Thank you!

@eng73 eng73 marked this pull request as ready for review July 3, 2022 12:24
@Paebbels Paebbels self-requested a review July 3, 2022 12:48
Copy link
Member

@Paebbels Paebbels left a comment

Choose a reason for hiding this comment

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

The printouts are only in pyGHDL.dom, so please move the logger instance to that package.
pyGHDL itself has no outputs and will never have them.

As all messages are swallowed by the logger, a printout in https://github.com/ghdl/ghdl/blob/master/pyGHDL/cli/dom.py is missing.

Copy link
Member

@Paebbels Paebbels left a comment

Choose a reason for hiding this comment

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

The given implementation change doesn't have the same behavior.

  • Messages do not print in order
    Your PR master branch
    image image
  • Messages from CLI are not shown at all.
    Your PR master branch
    image image

The purpose of pyGHDL.cli.dom is to test and demonstrate capabilities of pyGHDL.dom. So a printout of the translated structure is essential.

Also printing error messages at the point where they appear is also essential.

@@ -316,7 +317,7 @@ def HandlePretty(self, args):
for line in PP.formatDesign(self._design, 1):
buffer.append(line)

print("\n".join(buffer))
logger.info("\n".join(buffer))
Copy link
Member

Choose a reason for hiding this comment

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

This output is not printed by logger.

Copy link
Author

Choose a reason for hiding this comment

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

As this is a test application, I propose reverting these back to print statements.

@@ -118,7 +118,7 @@ def parse(cls, attributeNode: Iir) -> "AttributeSpecification":
if nameKind == nodes.Iir_Kind.Simple_Name:
names.append(SimpleName(name, GetNameOfNode(name)))
elif nameKind == nodes.Iir_Kind.Signature:
print("[NOT IMPLEMENTED] Signature name in attribute specifications.")
logger.warning("[NOT IMPLEMENTED] Signature name in attribute specifications.")
Copy link
Member

Choose a reason for hiding this comment

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

Messages don't print in-order because of logger.

@umarcor
Copy link
Member

umarcor commented Feb 25, 2023

Is ordering still a problem after the last update to this PR?

@Paebbels
Copy link
Member

Yes it is.

I see 2 solutions to the problem:

  1. we add a mode or similar to deactivate these printouts - note pyGHDL.dom is an alpha state extension - so it's not finished and fully ready for production, as features are missing.
  2. we remove print outs from code and keep it on a dev-branch for the developer(s).

Currently, print out are use to denote what's not translated to pyVHDLModel. Throwing an exception is no option, because it's not meant to be an exception and it would interrupt the translation, so almost no input could be translated.

Should I investigate if we can add a flag like "development mode" or so?

@umarcor
Copy link
Member

umarcor commented Feb 25, 2023

I think it makes sense to have a "debug" mode, which can be enabled by either a CLI option or an environment variable. Maybe GHDL_DEBUG=true (so it can potentially be used by ghdl itself)? Or just PYGHDL_DEBUG=true?

@Paebbels
Copy link
Member

I'll need some time for this change.

@umarcor
Copy link
Member

umarcor commented Feb 26, 2023

No rush 馃槈

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.

None yet

3 participants