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

BaseTools: Python VfrCompiler implementation #109

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

Conversation

yytshirley
Copy link

BaseTools: Python VfrCompiler implementation

This python VfrCompiler tool is the python implementation
of the edk2 VfrCompiler tool which C implementation locates at
https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/VfrCompile.

This python implementation not only covers the same usage as
the C version VfrCompiler, but also extends several new features.

Edk2 Basetools issue link:
#68

Cc: Rebecca Cran [email protected]
Cc: Liming Gao [email protected]
Cc: Bob Feng [email protected]
Signed-off-by: Yuting Yang [email protected]
Signed-off-by: Yuwei Chen [email protected]

This python VfrCompiler tool is the python implementation
of the edk2 VfrCompiler tool which C implementation locates at
https://github.com/tianocore/edk2/tree/master/BaseTools/Source/C/VfrCompile.

This python implementation not only covers the same usage as
the C version VfrCompiler, but also extends several new features.

Edk2 Basetools issue link:

Signed-off-by: Yuting Yang <[email protected]>
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Attention: 5738 lines in your changes are missing coverage. Please review.

Comparison is base (8d9b898) 4.27% compared to head (e1cfac4) 0.00%.

❗ Current head e1cfac4 differs from pull request most recent head 1cf69a5. Consider uploading reports for the commit 1cf69a5 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage    4.27%    0.00%   -4.27%     
==========================================
  Files         182      171      -11     
  Lines       81605   102585   +20980     
==========================================
- Hits         3490        8    -3482     
- Misses      78115   102577   +24462     
Flag Coverage Δ
Linux <0.01% <0.00%> (-4.27%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
edk2basetools/VfrCompiler/VfrSyntaxLexer.py 0.00% <ø> (ø)
edk2basetools/VfrCompiler/VfrSyntaxParser.py 0.00% <ø> (ø)
edk2basetools/VfrCompiler/VfrSyntaxVisitor.py 0.00% <ø> (ø)
edk2basetools/VfrCompiler/__init__.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrCommon.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrError.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrPreProcess.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrCompiler.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrTree.py 0.00% <0.00%> (ø)
edk2basetools/VfrCompiler/IfrCtypes.py 0.00% <0.00%> (ø)
... and 2 more

... and 32 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

@gahan9 gahan9 left a comment

Choose a reason for hiding this comment

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

Build files should be ignored from review commit.

from VfrCompiler.IfrCtypes import EFI_GUID
from Common.BuildToolError import PARAMETER_INVALID

# Enumeration of EFI_STATUS.
Copy link
Member

Choose a reason for hiding this comment

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

Many tools are going to need this. I would suggest figuring out a common way to do this. See https://github.com/tianocore/edk2-pytool-library/blob/master/edk2toollib/uefi/status_codes.py as an example.



# Converts a string to an EFI_GUID.
def StringToGuid(AsciiGuidBuffer: str, GuidBuffer: EFI_GUID):
Copy link
Member

Choose a reason for hiding this comment

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

for guid support we have found the uuid module works pretty good and avoids having to write python code that looks like C. :)
For some examples seem this search. https://github.com/search?q=repo%3Atianocore%2Fedk2-pytool-library+uuid&type=code

Choose a reason for hiding this comment

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

That's a good suggestion. Perhaps this feedback can be clubbed in the list of known issues Christine mentioned below? This will allow for upstreaming the present work to allow for more usage and learning from these improvements & feature additions. Sort of the 1.0->2.0->.. model of learning by shipping.

@YuweiChen1110
Copy link
Contributor

YuweiChen1110 commented Nov 7, 2023

Thanks for the feedbacks, I will add them to Known issues:

Known issues

  • Issue 1 - Test Coverage & Method
    • The current python vfrcompiler tool does not have a full code coverage test. Should add unit tests for higher code coverage.
    • The test should be enabled into Azure pipelines for retriggering.
    • The current test script use some output binaries for test input, will remove these binaries, and save the necessary part content of it directly into the test script itself.
  • Issue 2 - Code style
    • The current python vfrcompiler tool has some coding style still like C style which not follows the python standardize rule.
      • Should enhance GUID usage with 'uuid' pip module;
      • Should figure out common way for Status Code processing;
    • The current python vfrcompiler tool need docstring description for code explanation.
  • Issue 3 - New generated YAML format
    • The current YAML format is a reference version, and we warmly welcome everyone to provide feedback.
  • Future extension:
    • The tool will extend new functions, which is able to compile yaml files. This feature will be added in future update.

Since it will not block the basetools usage and will not immediately enabled into build, is that possible to merge it firstly, and solve these issues one by one in the future? It is important for extended features testing (If so, will update the Known issues into ReadMe file and update the patch with basetools's import path)

@vincent-j-zimmer
Copy link

Thanks for the feedbacks, I will add them to Known issues:

Known issues

  • Issue 1 - Test Coverage

    • The current python vfrcompiler tool does not have a full code coverage test. Should add unit tests for higher code coverage.
    • The test should be enabled into Azure pipelines for retriggering.
  • Issue 2 - Code style

    • The current python vfrcompiler tool has some coding style still like C style which not follows the python standardize rule.

      • Should enhance GUID usage with 'uuid' pip module;
      • Should figure out common way for Status Code processing;
    • The current python vfrcompiler tool need docstring description for code explanation.

  • Issue 3 - New generated YAML format

    • The current YAML format is a reference version, and we warmly welcome everyone to provide feedback.
  • Future extension:

    • The tool will extend new functions, which is able to compile yaml files. This feature will be added in future update.

Since it will not block the basetools usage and will not immediately enabled into build, is that possible to merge it firstly, and solve these issues one by one in the future? It is important for extended features testing (If so, will update the Known issues into ReadMe file and update the patch with basetools's import path)

This approach makes sense to me.

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.

5 participants