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

Allow build wrappers #341

Merged
merged 4 commits into from Nov 11, 2022
Merged

Conversation

makubacki
Copy link
Member

Closes #340

Adds two optional environment variables that, if specified, will be
used in lieu of the build command and its parameters during build.

  1. EDK_BUILD_CMD - If present, the absolute path to an application
    to use for the edk build process that will be invoked instead of
    build. This is primarily used to allow a tool to wrap around
    build such that it will eventually call build internally.

  2. EDK_BUILD_PARAMS - If present, these parameters will be passed
    to the build command. This is primarily used to pair wrapper-
    specific parameters with the wrapper passed in EDK_BUILD_CMD.

From a practical standpoint, these are the minimum changes needed
in the build tools to allow the CodeQL CLI to be invoked as a build
wrapper.

Signed-off-by: Michael Kubacki michael.kubacki@microsoft.com

@codecov
Copy link

codecov bot commented Nov 7, 2022

Codecov Report

Merging #341 (882910b) into master (958d682) will increase coverage by 1.21%.
The diff coverage is 77.77%.

❗ Current head 882910b differs from pull request most recent head 9f820c4. Consider uploading reports for the commit 9f820c4 to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #341      +/-   ##
==========================================
+ Coverage   70.42%   71.63%   +1.21%     
==========================================
  Files          48       48              
  Lines        4804     4812       +8     
==========================================
+ Hits         3383     3447      +64     
+ Misses       1421     1365      -56     
Flag Coverage Δ
Linux 70.80% <77.77%> (+1.21%) ⬆️
Windows_NT 71.57% <77.77%> (+1.21%) ⬆️

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

Impacted Files Coverage Δ
edk2toolext/environment/uefi_build.py 61.04% <77.77%> (+8.22%) ⬆️
edk2toolext/edk2_logging.py 78.14% <0.00%> (+16.39%) ⬆️

@Javagedes
Copy link
Contributor

Javagedes commented Nov 8, 2022

I agree this is a minimal change but I'm still hesitant as it's changing the core functionality of uefi_build (and in turn edk2_platform_build) to do something other than build the platform.

Have you considered creating a new invocable by subclassing the edk2_multipkg_aware_invocable? You'll be able to specify whatever platform specific information you need from the settings interface and then override the Go() method with your functionality to build the database. With it being separated like that, you could even add additional CLI flags to perform abstracted operations on the generated database.

@makubacki
Copy link
Member Author

I agree this is a minimal change but I'm still hesitant as it's changing the core functionality of uefi_build (and in turn edk2_platform_build) to do something other than build the platform.

Have you considered creating a new invocable by subclassing the edk2_multipkg_aware_invocable? You'll be able to specify whatever platform specific information you need from the settings interface and then override the Go() method with your functionality to build the database. With it being separated like that, you could even add additional CLI flags to perform abstracted operations on the generated database.

I considered a few alternatives like that but considered this to be the least invasive and easiest to use. For example, it can be activated with a couple lines of code: https://github.com/microsoft/mu_basecore/pull/163/files#diff-43c41ddc9a4e792cde552a99794d739893ff4d61c3c33f729816f069ac1c4498R108

It is easy to understand, reason about, and provides a higher degree of flexibility as the variables can be set based on many preconditions like various places in a settings file based on CLI flags or other criteria, from plugins (like in that example), etc.

The change is meant to be backward compatible. What's the concern about core functionality?

@Javagedes
Copy link
Contributor

Javagedes commented Nov 8, 2022

I'm not concerned with backwards compatibility or even from your technical implementation. This really is a simple change that doesn't affect anyone that doesn't know to override the environment variables.

My concern is stemming from the fact that we are evolving the purpose edk2_platform_build and uefi_build to do something other than solely build the platform.

Another concern I have is that this look incredibly hard to use for someone wanting to create a codeql database locally.

Can you show me an example of how this is used?

Based off what you did in your last PR to enable codeql, you were able to reuse the entire edk2 build command that gets generated by writing it to the command file and executing it. I don't see any ability to reuse the auto generated build command. So does person trying to create a codeql database have to create a file and manually add all of the EDK2 build command flags and the Codeql flags to it, then go in and run the command with stuart build?

i.e.

I have a command file containing the following (which the user somehow has to know to generate).

build -p QemuQ35Pkg/QemuQ35Pkg.dsc -b DEBUG -t VS2022 -a IA32 -a X64 -y C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\BUILD_REPORT.TXT -Y PCD -Y DEPEX -Y FLASH -Y BUILD_FLAGS -Y LIBRARY -Y FIXED_ADDRESS -Y HASH -D BUILDID_STRING=Unknown -D QEMU_CORE_NUM=2 -D SHIP_MODE=FALSE -D CONF_POLICY_GUID_REGISTRY=6E08E434-8E04-47B5-9A77-78A3A24523EA -D CONF_POLICY_GUID_BYTES='{0x34,0xE4,0x8,0x6E,0x4,0x8E,0xB5,0x47,0x9A,0x77,0x78,0xA3,0xA2,0x45,0x23,0xEA}' -D POLICY_BIN_PATH=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\Policy\secure_policy.bin -D CONF_BIN_FILE_0=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_0.bin -D CONF_BIN_FILE_1=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_1.bin -D CONF_BIN_FILE_2=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_2.bin -D CONF_BIN_FILE_3=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_3.bin

but then they also have to run stuart_build -c Platforms/QemuQ35Pkg/PlatformBuild.py EDK_BUILD_CMD=/path/to/codeql EDK_BUILD_PARAMS="database create /path/to/database --language=cpp --source-root=/path/to/root --command=/path/to/command_file".

Or am I just completely off the mark in how you we use codeql?

@makubacki
Copy link
Member Author

I think there's a few concepts getting mixed up here.

I'm not concerned with backwards compatibility or even from your technical implementation. This really is a simple change that doesn't affect anyone that doesn't know to override the environment variables.

My concern is stemming from the fact that we are evolving the purpose edk2_platform_build and uefi_build to do something other than solely build the platform.

Stuart itself is largely a wrapper around the build command. The build command interface is completely independent, well-defined, and used by many parties - directly, by stuart, and many other wrappers like stuart.

These pieces still build the platform. The only difference is that that they now have a hook for an additional wrapper. This is fundamentally how tools like CodeQL work - they need to wrap around the build (the real build not stuart operations which are a wrapper) to trace compilation output. Regardless of how things are moved around in stuart, CodeQL needs to trace the actual build output from build.

Another concern I have is that this look incredibly hard to use for someone wanting to create a codeql database locally.

CodeQL is independent of this change, but why do you think it is difficult?

It is very easy. An end user gets a database by running their normal stuart build commands.

Adding the plugin is also simple:

  1. Add codeql-build to the scopes returned by the build script
  2. Run stuart_build

Can you show me an example of how this is used?

        builder.env.SetValue(
            "EDK_BUILD_CMD", wrapper_path, "Set by X")
        builder.env.SetValue(
            "EDK_BUILD_PARAMS", wrapper_params, "Set by X")

Based off what you did in your last PR to enable codeql, you were able to reuse the entire edk2 build command that gets generated by writing it to the command file and executing it. I don't see any ability to reuse the auto generated build command. So does person trying to create a codeql database have to create a file and manually add all of the EDK2 build command flags and the Codeql flags to it, then go in and run the command with stuart build?

i.e.

I have a command file containing the following (which the user somehow has to know to generate).

build -p QemuQ35Pkg/QemuQ35Pkg.dsc -b DEBUG -t VS2022 -a IA32 -a X64 -y C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\BUILD_REPORT.TXT -Y PCD -Y DEPEX -Y FLASH -Y BUILD_FLAGS -Y LIBRARY -Y FIXED_ADDRESS -Y HASH -D BUILDID_STRING=Unknown -D QEMU_CORE_NUM=2 -D SHIP_MODE=FALSE -D CONF_POLICY_GUID_REGISTRY=6E08E434-8E04-47B5-9A77-78A3A24523EA -D CONF_POLICY_GUID_BYTES='{0x34,0xE4,0x8,0x6E,0x4,0x8E,0xB5,0x47,0x9A,0x77,0x78,0xA3,0xA2,0x45,0x23,0xEA}' -D POLICY_BIN_PATH=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\Policy\secure_policy.bin -D CONF_BIN_FILE_0=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_0.bin -D CONF_BIN_FILE_1=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_1.bin -D CONF_BIN_FILE_2=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_2.bin -D CONF_BIN_FILE_3=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_3.bin

but then they also have to run stuart_build -c Platforms/QemuQ35Pkg/PlatformBuild.py EDK_BUILD_CMD=/path/to/codeql EDK_BUILD_PARAMS="database create /path/to/database --language=cpp --source-root=/path/to/root --command=/path/to/command_file".

Or am I just completely off the mark in how you we use codeql?

That's not how it works.

As mentioned earlier, the end user does nothing other than run normal build commands.

I suggest reading the High-Level Operation of the CodeQL Plugin readme to understand what the plugins do. Keep in mind, that's what the plugins do, not what a user does.

Regarding the plugin internals around how the build parameters are created:

  • The build parameters are produced in the _get_build_params(self) function of CodeQlBuildPlugin.py
  • The full build command is written to a file in the following line of code in CodeQlBuildPlugin.py:
self.codeql_cmd_path.write_text(encoding='utf8', data=codeql_build_cmd)

So, the file simply contains build ... parameters.

That file is referenced as the "build file" for CodeQL in the following code in CodeQlBuildPlugin.py (--command parameter):

        codeql_params = (f'database create {self.codeql_db_path} '
                            f'--language=cpp '
                            f'--source-root={builder.ws} '
                            f'--command={self.codeql_cmd_path}')

That means CodeQL will call that file as the build command for it to trace.

The CodeQL executable (discovered by the plugin) and the CodeQL parameters (generated by the plugin - codeql_params above) are passed as these variables:

        builder.env.SetValue(
            "EDK_BUILD_CMD", self.codeql_path, "Set in CodeQL Build Plugin")
        builder.env.SetValue(
            "EDK_BUILD_PARAMS", codeql_params, "Set in CodeQL Build Plugin")

But, again, the point of the plugin is to automate all of this, so it is easy for an end user. Al the user does is turn on the scopes needed (as described in the plugin Readme.md) and build.

@Javagedes
Copy link
Contributor

I think there's a few concepts getting mixed up here.

I'm not concerned with backwards compatibility or even from your technical implementation. This really is a simple change that doesn't affect anyone that doesn't know to override the environment variables.
My concern is stemming from the fact that we are evolving the purpose edk2_platform_build and uefi_build to do something other than solely build the platform.

Stuart itself is largely a wrapper around the build command. The build command interface is completely independent, well-defined, and used by many parties - directly, by stuart, and many other wrappers like stuart.

These pieces still build the platform. The only difference is that that they now have a hook for an additional wrapper. This is fundamentally how tools like CodeQL work - they need to wrap around the build (the real build not stuart operations which are a wrapper) to trace compilation output. Regardless of how things are moved around in stuart, CodeQL needs to trace the actual build output from build.

Another concern I have is that this look incredibly hard to use for someone wanting to create a codeql database locally.

CodeQL is independent of this change, but why do you think it is difficult?

It is very easy. An end user gets a database by running their normal stuart build commands.

Adding the plugin is also simple:

  1. Add codeql-build to the scopes returned by the build script
  2. Run stuart_build

Can you show me an example of how this is used?

        builder.env.SetValue(
            "EDK_BUILD_CMD", wrapper_path, "Set by X")
        builder.env.SetValue(
            "EDK_BUILD_PARAMS", wrapper_params, "Set by X")

Based off what you did in your last PR to enable codeql, you were able to reuse the entire edk2 build command that gets generated by writing it to the command file and executing it. I don't see any ability to reuse the auto generated build command. So does person trying to create a codeql database have to create a file and manually add all of the EDK2 build command flags and the Codeql flags to it, then go in and run the command with stuart build?
i.e.
I have a command file containing the following (which the user somehow has to know to generate).
build -p QemuQ35Pkg/QemuQ35Pkg.dsc -b DEBUG -t VS2022 -a IA32 -a X64 -y C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\BUILD_REPORT.TXT -Y PCD -Y DEPEX -Y FLASH -Y BUILD_FLAGS -Y LIBRARY -Y FIXED_ADDRESS -Y HASH -D BUILDID_STRING=Unknown -D QEMU_CORE_NUM=2 -D SHIP_MODE=FALSE -D CONF_POLICY_GUID_REGISTRY=6E08E434-8E04-47B5-9A77-78A3A24523EA -D CONF_POLICY_GUID_BYTES='{0x34,0xE4,0x8,0x6E,0x4,0x8E,0xB5,0x47,0x9A,0x77,0x78,0xA3,0xA2,0x45,0x23,0xEA}' -D POLICY_BIN_PATH=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\Policy\secure_policy.bin -D CONF_BIN_FILE_0=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_0.bin -D CONF_BIN_FILE_1=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_1.bin -D CONF_BIN_FILE_2=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_2.bin -D CONF_BIN_FILE_3=C:\src\mu_tiano_platforms\Build\QemuQ35Pkg\DEBUG_VS2022\ConfPolicy\ConfPolicyVarBin_3.bin
but then they also have to run stuart_build -c Platforms/QemuQ35Pkg/PlatformBuild.py EDK_BUILD_CMD=/path/to/codeql EDK_BUILD_PARAMS="database create /path/to/database --language=cpp --source-root=/path/to/root --command=/path/to/command_file".
Or am I just completely off the mark in how you we use codeql?

That's not how it works.

As mentioned earlier, the end user does nothing other than run normal build commands.

I suggest reading the High-Level Operation of the CodeQL Plugin readme to understand what the plugins do. Keep in mind, that's what the plugins do, not what a user does.

Regarding the plugin internals around how the build parameters are created:

  • The build parameters are produced in the _get_build_params(self) function of CodeQlBuildPlugin.py
  • The full build command is written to a file in the following line of code in CodeQlBuildPlugin.py:
self.codeql_cmd_path.write_text(encoding='utf8', data=codeql_build_cmd)

So, the file simply contains build ... parameters.

That file is referenced as the "build file" for CodeQL in the following code in CodeQlBuildPlugin.py (--command parameter):

        codeql_params = (f'database create {self.codeql_db_path} '
                            f'--language=cpp '
                            f'--source-root={builder.ws} '
                            f'--command={self.codeql_cmd_path}')

That means CodeQL will call that file as the build command for it to trace.

The CodeQL executable (discovered by the plugin) and the CodeQL parameters (generated by the plugin - codeql_params above) are passed as these variables:

        builder.env.SetValue(
            "EDK_BUILD_CMD", self.codeql_path, "Set in CodeQL Build Plugin")
        builder.env.SetValue(
            "EDK_BUILD_PARAMS", codeql_params, "Set in CodeQL Build Plugin")

But, again, the point of the plugin is to automate all of this, so it is easy for an end user. Al the user does is turn on the scopes needed (as described in the plugin Readme.md) and build.

This confusion stemmed from the fact I was unaware you created a plug-in to go along with this change. I will re-review this along with the plug-in in the morning.

docs/usability/environment_variables.md Outdated Show resolved Hide resolved
@Javagedes Javagedes added the enhancement New feature or request label Nov 10, 2022
@Javagedes Javagedes added this to the 0.20.0 milestone Nov 10, 2022
Closes tianocore#340

Adds two optional environment variables that, if specified, will be
used in lieu of the `build` command and its parameters during build.

1. `EDK_BUILD_CMD` - If present, the absolute path to an application
   to use for the edk build process that will be invoked instead of
   `build`. This is primarily used to allow a tool to wrap around
   `build` such that it will eventually call `build` internally.

2. `EDK_BUILD_PARAMS` - If present, these parameters will be passed
   to the build command. This is primarily used to pair wrapper-
   specific parameters with the wrapper passed in `EDK_BUILD_CMD`.

From a practical standpoint, these are the minimum changes needed
in the build tools to allow the CodeQL CLI to be invoked as a build
wrapper.

Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
@Javagedes Javagedes merged commit bb4e9ef into tianocore:master Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Allow build wrappers
2 participants