Create crypto binaries report #122

DorLevi95 · 2025-01-12T10:43:31Z

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

inbal2l · 2025-01-13T11:50:11Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+
+    def get_module_type_for_crypto_bin(self, thebuilder):
+
+        CryptoBinPkg_Driver_path = Path(thebuilder.ws) / "CryptoBinPkg" / "Driver"


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.

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

Flickdm

Thanks Dor! This looks great and super useful!

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

SingleFlavorBuild.py

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

Flickdm · 2025-01-16T19:38:12Z

.azuredevops/CryptoTest.yml

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


Do we need to save the Release version as well?

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?

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

Flickdm · 2025-01-16T19:43:22Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+
+                    # 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)


Maybe throw an exception if this assumption is invalid

Flickdm · 2025-01-16T19:44:31Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+                # 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")


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

Flickdm · 2025-01-16T19:46:24Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

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


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?

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

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?

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

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

Flickdm · 2025-01-30T20:20:44Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

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


nit: lowercase :)

Flickdm · 2025-01-30T20:21:21Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

@@ -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):


nit snakecase / lowercase :)

DorLevi95 · 2025-02-06T12:52:54Z

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

Javagedes · 2025-03-05T16:56:09Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto_plug_in.json

+    "scope": "crypto-report",
+    "name": "Report Crypto Contents",
+    "module": "ReportCrypto"
+}


nit: newline

Javagedes · 2025-03-05T16:57:02Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

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


Javagedes · 2025-03-05T17:02:14Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+    - OpenSSL library flags
+    Methods:
+    --------
+    do_post_build(thebuilder):


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

Javagedes · 2025-03-05T17:03:02Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+    """
+
+    def do_post_build(self, thebuilder):
+        ### Initiazlize variables ###


nit: spellcheck Initiazlize -> Initialize

Javagedes · 2025-03-05T17:04:09Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+        ### Initiazlize variables ###
+
+        # get needed paths
+        openssl_lib_path = Path(thebuilder.edk2path.GetAbsolutePathOnThisSystemFromEdk2RelativePath("OpensslPkg")) / "Library" / "OpensslLib"


Suggested change

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

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

Javagedes · 2025-03-05T17:06:38Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

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


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.

Javagedes · 2025-03-05T17:07:25Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+                    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")   


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

Javagedes · 2025-03-05T17:09:52Z

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py

+
+    def get_linked_lib(self, arch, module_type, lib, cryptoBinPkg_dsc_path):
+
+        with cryptoBinPkg_dsc_path.open() as f:


Why are you not using our DSC parser?

https://github.com/tianocore/edk2-pytool-library/blob/46601327615e6761ffaf83b57398485965af0749/edk2toollib/uefi/edk2/parsers/dsc_parser.py#L18

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

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.

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 · 2025-03-06T13:26:59Z

@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 · 2025-03-06T19:20:41Z

@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!

github-actions · 2025-05-08T23:39:46Z

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 · 2025-07-08T23:40:22Z

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 bot added the language:python Pull requests that update Python code label Jan 12, 2025

DorLevi95 changed the title ~~Create build report about the crypto binaries~~ Create crypto binaries report Jan 12, 2025

inbal2l reviewed Jan 13, 2025

View reviewed changes

CryptoBinPkg/Plugin/ReportCrypto/ReportCrypto.py Show resolved Hide resolved

Flickdm self-requested a review January 13, 2025 18:10

Flickdm requested changes Jan 16, 2025

View reviewed changes

Flickdm requested review from kenlautner and makubacki January 16, 2025 19:58

Flickdm reviewed Jan 30, 2025

View reviewed changes

Flickdm approved these changes Jan 30, 2025

View reviewed changes

DorLevi95 force-pushed the feature/crypto_binaries_report branch from f3fa744 to d5b554a Compare February 6, 2025 12:51

DorLevi95 changed the base branch from release/202311 to release/202405-dev February 6, 2025 12:52

DorLevi95 added 7 commits February 9, 2025 12:19

create a post build report to report built binaries info

6d26172

create and publish report in CryptoTest pipeline

2e8c332

centralize paths initializations

814e03c

fix offset alignment

32c361b

add docs for class and methods

6c45d00

undo the use of global variables, pass them as params

53ddf36

fix typo

8b582fb

DorLevi95 force-pushed the feature/crypto_binaries_report branch from d5b554a to 8b582fb Compare February 9, 2025 10:22

Javagedes reviewed Mar 5, 2025

View reviewed changes

nit: remove unnecessary comments, typos, etc

155d5de

use DscParser

c003a9c

github-actions bot added the state:stale Has not been updated in a long time label May 8, 2025

apop5 removed the state:stale Has not been updated in a long time label May 9, 2025

github-actions bot added the state:stale Has not been updated in a long time label Jul 8, 2025


		def get_module_type_for_crypto_bin(self, thebuilder):

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

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


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

		with cryptoBinPkg_dsc_path.open() as f:

Create crypto binaries report #122

Are you sure you want to change the base?

Create crypto binaries report #122

Conversation

DorLevi95 commented Jan 12, 2025

Description

Uh oh!

inbal2l Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Flickdm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DorLevi95 commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Javagedes Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Javagedes Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Javagedes left a comment

Choose a reason for hiding this comment

Uh oh!

DorLevi95 commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Javagedes commented Mar 6, 2025

Uh oh!

github-actions bot commented May 8, 2025

Uh oh!

github-actions bot commented Jul 8, 2025

Uh oh!

Uh oh!

inbal2l Jan 13, 2025 •

edited

Loading

Javagedes Mar 5, 2025 •

edited

Loading

Javagedes Mar 5, 2025 •

edited

Loading

DorLevi95 commented Mar 6, 2025 •

edited

Loading