-
Notifications
You must be signed in to change notification settings - Fork 16
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
base: release/202405-dev
Are you sure you want to change the base?
Create crypto binaries report #122
Conversation
|
||
def get_module_type_for_crypto_bin(self, thebuilder): | ||
|
||
CryptoBinPkg_Driver_path = Path(thebuilder.ws) / "CryptoBinPkg" / "Driver" |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit snakecase / lowercase :)
f3fa744
to
d5b554a
Compare
@Flickdm @inbal2l @kenlautner @makubacki Rebased and changed target branch to release/202405-dev |
d5b554a
to
8b582fb
Compare
"scope": "crypto-report", | ||
"name": "Report Crypto Contents", | ||
"module": "ReportCrypto" | ||
} |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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 ### |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are you not using our DSC parser?
There was a problem hiding this comment.
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
There was a problem hiding this 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)
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. 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! |
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. |
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. |
Description
Creating a reporting script to run in post build.
This script will generate a report about the crypto binaries containing useful info:
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