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
Allow build wrappers #341
Conversation
Codecov Report
Additional details and impacted files@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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? |
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).
but then they also have to run Or am I just completely off the mark in how you we use codeql? |
I think there's a few concepts getting mixed up here.
Stuart itself is largely a wrapper around the 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
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:
builder.env.SetValue(
"EDK_BUILD_CMD", wrapper_path, "Set by X")
builder.env.SetValue(
"EDK_BUILD_PARAMS", wrapper_params, "Set by X")
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
self.codeql_cmd_path.write_text(encoding='utf8', data=codeql_build_cmd) So, the file simply contains That file is referenced as the "build file" for CodeQL in the following code in CodeQlBuildPlugin.py ( 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 - 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. |
393f8e4
to
de8b421
Compare
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>
7006a97
to
9f820c4
Compare
Closes #340
Adds two optional environment variables that, if specified, will be
used in lieu of the
build
command and its parameters during build.EDK_BUILD_CMD
- If present, the absolute path to an applicationto use for the edk build process that will be invoked instead of
build
. This is primarily used to allow a tool to wrap aroundbuild
such that it will eventually callbuild
internally.EDK_BUILD_PARAMS
- If present, these parameters will be passedto 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