Skip to content

Create crypto binaries report #122

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

Open
wants to merge 9 commits into
base: release/202405-dev
Choose a base branch
from

Conversation

DorLevi95
Copy link
Collaborator

Description

Creating a reporting script to run in post build.
This script will generate a report about the crypto binaries containing useful info:

  • Binaries sizes
  • Submodules info
  • Linked openssllib*.inf to each binary
  • Openssllib*.inf build configuration flags

The report will be placed in the build folder with the name "Report_" and published during Crypto tests pipeline.
A follow up PR needs to be added, which will compare reports during each PR (latest release branch report as the baseline report) and produce necessary warnings.
The goal is to catch risky changes that can break functionality or influence the memory consumption (e.g. increase or decrease in size).

Currently the report is only generated and publish during the Crypto tests pipeline. i.e. STANDARD_DEBUG flavor.
This mainly influences only the binaries sizes and might be enough to catch abnormal increase/decrease.

Please refer to this draft PR to view and example of the report and its comparison during a PR: #118

@github-actions github-actions bot added the language:python Pull requests that update Python code label Jan 12, 2025
@DorLevi95 DorLevi95 changed the title Create build report about the crypto binaries Create crypto binaries report Jan 12, 2025

def get_module_type_for_crypto_bin(self, thebuilder):

CryptoBinPkg_Driver_path = Path(thebuilder.ws) / "CryptoBinPkg" / "Driver"
Copy link

@inbal2l inbal2l Jan 13, 2025

Choose a reason for hiding this comment

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

Would probebly define CryptoBinPkg_Driver_path (along side other enrvironment-dependent params) in the beginning of the program to make it visible, so that if changed can be changed in one place.

@Flickdm Flickdm self-requested a review January 13, 2025 18:10
Copy link
Member

@Flickdm Flickdm left a comment

Choose a reason for hiding this comment

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

Thanks Dor! This looks great and super useful!

@@ -158,6 +159,7 @@ stages:
BUILDLOG_CryptoBin_*.txt
SETUPLOG.txt
UPDATE_LOG.txt
Report_STANDARD_DEBUG_VS2022.txt
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to save the Release version as well?

Copy link
Member

Choose a reason for hiding this comment

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

report_file_name = f"Report_{thebuilder.flavor}_{target}_{tool_chain}.txt"

This would appear to suggest that we need to handle flavor, target, tool_chain?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The report can be created for each of these 3 configurations (using --report flag)
I triggered it in the Crypto Test pipeline which currently just builds this specific combination.
Besides of the binary sizes, nothing else in the report is different between different flavors, target, toolchain


# Get the number of k bytes of the compressed data
compressed_data_size = len(compressed_data) / 1024
# log to file (assuming file name won't be longer than 40 characters to keep the report clean)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe throw an exception if this assumption is invalid

# get module type for the binary
module_type = binary_to_type.get(file.name, "UEFI_APPLICATION") # Default to UEFI_APPLICATION if not found (e.g. test binary)
opensslib = self.get_linked_lib(thebuilder, arch, module_type, "OpensslLib")
report.append(f"{file.name} - " + (40-len(file.name))* " " + f"OpensslLib: {opensslib}\n")
Copy link
Member

Choose a reason for hiding this comment

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

Same assumption that the file name won't be longer than 40 characters

openssl_inf_files.sort()
for file in openssl_inf_files:
# write openssl library files to report file
openssl_flags = self.get_openssl_flags(file)
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Would it be possible to map the flags to a human readable form? Or do you think that these would change / introduce too much complexity?

Copy link
Member

Choose a reason for hiding this comment

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

It could be interesting if we have a list of "required" flags and if one is missing that it throws an exception

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first suggestion sounds to indeed introduce more complexity. We would need to create a mapping between all the possible flags and their human read descriptions.
Most flags can be understood for their name.

The second suggestion sounds good.
The future comparison too is the one who aims to throw warnings for each change in the flags found in PR.
I don't know if we would want to change in to throw error OR throw it already in the reporting tool. As this would fail the build/pipeline (depends on which tool), and that might be what the user intended.

Moreover, if we would go down to it, do we have such pre-defined required flags?

Copy link
Member

Choose a reason for hiding this comment

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

I think so? Maybe not explicitly. For example, since we know we need Elliptic Curve support if it's ever missing that's would be problematic


# get needed paths
openssl_lib_path = Path(thebuilder.edk2path.GetAbsolutePathOnThisSystemFromEdk2RelativePath("OpensslPkg")) / "Library" / "OpensslLib"
CryptoBinPkg_Driver_path = Path(thebuilder.ws) / "CryptoBinPkg" / "Driver"
Copy link
Member

Choose a reason for hiding this comment

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

nit: lowercase :)

@@ -137,9 +171,8 @@ def get_module_type_for_crypto_bin(self, thebuilder):
binary_to_type[f"{base_name}.efi"] = module_type # start binaries sizes report
break

def get_linked_lib(self, thebuilder, arch, module_type, lib):
def get_linked_lib(self, arch, module_type, lib, cryptoBinPkg_dsc_path):
Copy link
Member

Choose a reason for hiding this comment

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

nit snakecase / lowercase :)

@DorLevi95 DorLevi95 force-pushed the feature/crypto_binaries_report branch from f3fa744 to d5b554a Compare February 6, 2025 12:51
@DorLevi95 DorLevi95 changed the base branch from release/202311 to release/202405-dev February 6, 2025 12:52
@DorLevi95
Copy link
Collaborator Author

@Flickdm @inbal2l @kenlautner @makubacki Rebased and changed target branch to release/202405-dev

@DorLevi95 DorLevi95 force-pushed the feature/crypto_binaries_report branch from d5b554a to 8b582fb Compare February 9, 2025 10:22
"scope": "crypto-report",
"name": "Report Crypto Contents",
"module": "ReportCrypto"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: newline

# Plugin to Bundle the CryptoBin build output
#
##
# Copyright (c) Microsoft Corporation. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

We no longer use All rights reserved.

- OpenSSL library flags
Methods:
--------
do_post_build(thebuilder):
Copy link
Contributor

@Javagedes Javagedes Mar 5, 2025

Choose a reason for hiding this comment

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

function documentation must be associated with the method itself, not in the overall class documentation. Here is a random example: https://github.com/tianocore/edk2-pytool-extensions/blob/26f14583fbfbbe9736fe941147ed1a791b8b7ffd/edk2toolext/environment/plugin_manager.py#L41

"""

def do_post_build(self, thebuilder):
### Initiazlize variables ###
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spellcheck Initiazlize -> Initialize

### Initiazlize variables ###

# get needed paths
openssl_lib_path = Path(thebuilder.edk2path.GetAbsolutePathOnThisSystemFromEdk2RelativePath("OpensslPkg")) / "Library" / "OpensslLib"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
openssl_lib_path = Path(thebuilder.edk2path.GetAbsolutePathOnThisSystemFromEdk2RelativePath("OpensslPkg")) / "Library" / "OpensslLib"
openssl_lib_path = Path(thebuilder.edk2path.GetAbsolutePathOnThisSystemFromEdk2RelativePath("OpensslPkg", "Library", "OpensslLib"))

time = datetime.now().strftime('%Y-%m-%d %H:%M:%S\n')
report = [title, "Build Time: " + time]

# get tool chain
Copy link
Contributor

@Javagedes Javagedes Mar 5, 2025

Choose a reason for hiding this comment

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

nit: A lot of these comments are unnecessary. If you need a comment for one or two lines of code, either you don't need the comment, or you need to make your code more readable. I'll leave it up to you to clean up some of the comments.

report.append(f"{file.name} - " + (offset-len(file.name))* " " + f"Uncompressed: {file_size:.2f} KB | LZMA Compressed: {compressed_data_size:.2f} KB\n")

# get linked openssl configuration
report.append("\n")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: just add this "\n" to the front of the next append.


def get_linked_lib(self, arch, module_type, lib, cryptoBinPkg_dsc_path):

with cryptoBinPkg_dsc_path.open() as f:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, I wasn't aware of it.
Used it in latest commit

Copy link
Contributor

@Javagedes Javagedes left a comment

Choose a reason for hiding this comment

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

Overall, why are we using this custom report format? This does not seem like a format that can be easily consumed by other tooling in the long run. Should it be? Should it be a json format of some sort? Yaml? Or is this only meant to be human readable? Will anyone ever decide that we want it to be consumed by other tools? if that last question even has a chance of being yes, then you should not use a custom format.

We have an example of this going wrong already - the EDKII BUILD_REPORT. We had to write a custom parser for a human readable report and it is quite finicky. I would like to avoid having to do this again. (https://github.com/microsoft/mu_basecore/blob/b077d2f2d402918f79be447dbbcf37234b0db0c4/BaseTools/Plugin/FdSizeReport/FdSizeReportGenerator.py#L114-L115)

@DorLevi95
Copy link
Collaborator Author

DorLevi95 commented Mar 6, 2025

@Javagedes

Overall, why are we using this custom report format? This does not seem like a format that can be easily consumed by other tooling in the long run. Should it be? Should it be a json format of some sort? Yaml? Or is this only meant to be human readable? Will anyone ever decide that we want it to be consumed by other tools? if that last question even has a chance of being yes, then you should not use a custom format.

It is mainly meant to be human readable + the tool that should consume this report is a comparison tool, meant to compare between 2 reports in Pull Requests and warn about risky changes between source and target branches.
I presented the overall solution in this draft PR: #118 containing both the report tool and the comparison tool (comparing with a test report).

This PR only suggests the report first, as it would first need to produce a report in the release branches to be later consumed in the PRs (as the target branch) for the comparison.

@Javagedes
Copy link
Contributor

@Javagedes

Overall, why are we using this custom report format? This does not seem like a format that can be easily consumed by other tooling in the long run. Should it be? Should it be a json format of some sort? Yaml? Or is this only meant to be human readable? Will anyone ever decide that we want it to be consumed by other tools? if that last question even has a chance of being yes, then you should not use a custom format.

It is mainly meant to be human readable + the tool that should consume this report is a comparison tool, meant to compare between 2 reports in Pull Requests and warn about risky changes between source and target branches. I presented the overall solution in this draft PR: #118 containing both the report tool and the comparison tool (comparing with a test report).

This PR only suggests the report first, as it would first need to produce a report in the release branches to be later consumed in the PRs (as the target branch) for the comparison.

OK This sounds good. Just wanted to make sure it was well thought out for creating a custom report format instead of using a standard format like json. Thanks for the explanation!

Copy link

github-actions bot commented May 8, 2025

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the state:stale Has not been updated in a long time label May 8, 2025
@apop5 apop5 removed the state:stale Has not been updated in a long time label May 9, 2025
Copy link

github-actions bot commented Jul 8, 2025

This PR has been automatically marked as stale because it has not had activity in 60 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions.

@github-actions github-actions bot added the state:stale Has not been updated in a long time label Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
language:python Pull requests that update Python code state:stale Has not been updated in a long time
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants