-
Notifications
You must be signed in to change notification settings - Fork 71
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
feat: enable CI workflows to use NuGet WDK packages #236
base: main
Are you sure you want to change the base?
Conversation
…getWDKContentRoot env variable
… tools/ for WDK tool root
crates/wdk-build/src/utils.rs
Outdated
@@ -98,6 +98,19 @@ pub fn detect_wdk_content_root() -> Option<PathBuf> { | |||
); | |||
} | |||
|
|||
// If NugetWdkContentRoot is present in environment, use it | |||
if let Ok(wdk_content_root) = env::var("NugetWdkContentRoot") { |
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 don't think we should be doing custom handling here for nuget since NugetWdkContentRoot
doesn't exist normally for the Kit. How does building with nuget generally work in a non-rust environment?
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 understand. The C samples use the Build-SampleSet.ps1 script. Adding the NugetWdkContentRoot
was one of the ways to point to the NuGet pkg root. I'm not fully aware of how msbuild works with NuGet pkgs and how SxS WDK installations work.
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 very much do not want to add new scaffolding to windows-drivers-rs when existing mechanisms exist. In order to make sure this doesn't diverge from the original kit, I think you need to find out how C/C++ WDK handles pointing to the WDKContentRoot when using the new nuget install methods. For eWDK, we check for the env vars that eWDK prompts set. For installed WDK, we look for the reg keys that the WDK installer sets. For nuget, we should use whatever mechanism DDX has for nuget for root discovery. @NateD-MSFT do you have insight/contacts for this?
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'll spin up a thread with the WDK team to ask.
.github/workflows/build.yaml
Outdated
@@ -20,6 +20,7 @@ jobs: | |||
matrix: | |||
wdk: | |||
- Microsoft.WindowsWDK.10.0.22621 # NI WDK | |||
- Microsoft.Windows.WDK # Latest WDK |
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.
Even with nuget, we want to be able to test specific versions of the kit
…d_length_path_prefix
…into port-pipeline
…1 script to install WDK version based on the matrix input, fix clippy issue
…_or issue in utils.rs
WDF_DRIVER_CONFIG, | ||
WDF_NO_HANDLE, | ||
WDF_NO_OBJECT_ATTRIBUTES, | ||
call_unsafe_wdf_function_binding, ntddk::DbgPrint, DRIVER_OBJECT, NTSTATUS, PCUNICODE_STRING, PDRIVER_OBJECT, ULONG, UNICODE_STRING, WCHAR, WDFDEVICE, WDFDEVICE_INIT, WDFDRIVER, WDF_DRIVER_CONFIG, WDF_NO_HANDLE, WDF_NO_OBJECT_ATTRIBUTES |
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.
Not a blocker, but I am curious how this reformat wasn't caught sooner by CI/other pipeline checkins.
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 this change actually needs to be reverted? I suspect that whats going on is that:
- cargo fmt check is only run at repo root. AFAIU, it traverses files it sees based on cargo-metadata (ie. it only looks in the workspace)
- the mixed-package-kmdf-workspace is actually its own cargo workspace, so that cargo fmt has no idea it exists
There are some gaps in the top-level examples and tests folders where certain pipelines are not fully covering them. spent some time figuring out what gaps there are (mostly due to changes related to #186 ) and will be adding the rundown to github in a pr
<package id="Microsoft.Windows.SDK.CPP.arm64" version="10.0.26100.1591" targetFramework="native" /> | ||
<package id="Microsoft.Windows.WDK.x64" version="10.0.26100.1591" targetFramework="native" /> | ||
<package id="Microsoft.Windows.WDK.arm64" version="10.0.26100.1591" targetFramework="native" /> | ||
</packages> |
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 at end of file is missing
# - 10.0.26100.1882 | ||
# - 10.0.26100.1591 |
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.
These are just QFE versions right? I think either have all of the QFE versions enabled, or remove the comment
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.
Also why not use the latest QFE here, if there is only going to be one
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 the best thing to do here is actually leave off the QFE version and get the latest QFE for a given build number
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.
FWIW the varying QFEs here do refer to meaningfully different builds of the WDK now that we've moved to a CD model. It is probably fine to just use the latest GE CD kit for the matrix rather than run each one.
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.
So for a given build number, a user should always be using the latest QFE, right?
FWIW the varying QFEs here do refer to meaningfully different builds of the WDK now that we've moved to a CD model. It is probably fine to just use the latest GE CD kit for the matrix rather than run each one.
Is this WRT to GE only, or do you think its always fine to just run on the latest QFE for every build number(including vNext wdk)? my intuition here is to have the matrix always run:
- the latest QFE for every build number
- both winget and nuget since they use meaningfully different ways of exposing where its contents are (ie. regkey for winget and vcxproj for nuget, but per my other comment, i think we should piggyback off the ewdks' env var)
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 believe that prior to Ge there was no meaningful difference between QFEs. Kits almost exclusively shipped once per release. I think there was a special case in one release, but the concept of "multiple kits with the same build number but different QFEs, with different content" is new to Ge and CD.
@@ -20,6 +20,10 @@ jobs: | |||
matrix: | |||
wdk: |
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.
Rather than special casing the NI WDK, I think you should explicityly add another matrix dimension for WDK source. Then you can exclude the WDK versions from nuget that aren't available via the exclude feature. It would look something like:
matrix:
wdk_source: [nuget, winget]
wdk_version:
- 10.0.22621 # NI WDK
- 10.0.26100 #GE WDK
exclude:
# 10.0.22621 not available on nuget
- wdk_source: nuget
wdk_version: 10.0.22621
This way, your later steps can switch on if: matrix.wdk_source == nuget
and similar. The Nuget and Winget specific steps should be able to eaisly reconstruct the full install version string from just the wdk_version
run: | | ||
$wdk = '${{ matrix.wdk }}' | ||
if ($wdk -match '^10\.0\.\d{5}\.\d{1,4}$') { | ||
./.github/scripts/nuget-install-wdk.ps1 -wdkVersion $wdk |
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.
Needing to run this ps script seems a little hacky. Is there a reason you can't just nuget install
the required packages in this run block?
$packagesConfigPath = ".\packages.config" | ||
|
||
if (Test-Path $packagesConfigPath) { | ||
[xml]$packagesConfig = Get-Content $packagesConfigPath |
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.
rather than updating a version thats hardcoded into a packages.config (that is only used in CI), can you just use nuget install
in the action step?
exit 1 | ||
} | ||
|
||
$folders = @( |
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 is there all this manual copying of folders after the nuget packages are restored? this seems very brittle
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 setup_path
function sets the bin
and tools
root W.R.T standard WDK's directory structure. However, with NuGet I noticed that some bins and headers were placed in a different path. So, this script moves them under the root that setup_path
is using.
pub fn setup_path() -> Result<impl IntoIterator<Item = String>, ConfigError> { |
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.
this function actually uses detect_wdk_content_root
, so as long as that's detecting nuget properly, then we're all set.
see #236 (comment) for more details, but the tl;dr is that the nuget WDK's use in c drivers works by setting the include and bin paths via modifying the vcxproj. as stated in my linked comment, since there is no path forward to use the same solution that nuget uses in c drivers (ie. vcxproj is not supported by our rust tooling), we should rely on the env vars used by ewdk
@svasista-ms I did some of my own investigation on how C drivers use the WDK via nuget. It looks like when the nuget dependency is added via visual studio ui, the WDK's props and targets are added to the projects diff --git a/KMDF Driver1/KMDF Driver1.vcxproj b/KMDF Driver1/KMDF Driver1.vcxproj
index 6a4e8cf..8694853 100644
--- a/KMDF Driver1/KMDF Driver1.vcxproj
+++ b/KMDF Driver1/KMDF Driver1.vcxproj
@@ -1,5 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+ <Import Project="..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props')" />
<ItemGroup Label="ProjectConfigurations">
<ProjectConfiguration Include="Debug|x64">
<Configuration>Debug</Configuration>
(1/3) Stash this hunk [y,n,q,a,d,j,J,g,/,e,p,?]? y
@@ -19,6 +22,7 @@
</ProjectConfiguration>
</ItemGroup>
<ItemGroup>
+ <None Include="packages.config" />
<None Include="ReadMe.txt" />
</ItemGroup>
<ItemGroup>
(2/3) Stash this hunk [y,n,q,a,d,K,j,J,g,/,e,p,?]? y
@@ -147,5 +151,15 @@
</ItemGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets')" />
</ImportGroup>
+ <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
+ <PropertyGroup>
+ <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
+ </PropertyGroup>
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props'))" />
+ </Target>
</Project>
\ No newline at end of file
(3/3) Stash this hunk [y,n,q,a,d,K,g,/,s,e,p,?]? y
diff --git a/KMDF Driver1/KMDF Driver1.vcxproj b/KMDF Driver1/KMDF Driver1.vcxproj
index 6a4e8cf..8694853 100644
--- a/KMDF Driver1/KMDF Driver1.vcxproj
+++ b/KMDF Driver1/KMDF Driver1.vcxproj
@@ -1,5 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+ <Import Project="..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsof:...skipping...
diff --git a/KMDF Driver1/KMDF Driver1.vcxproj b/KMDF Driver1/KMDF Driver1.vcxproj
index 6a4e8cf..8694853 100644
--- a/KMDF Driver1/KMDF Driver1.vcxproj
+++ b/KMDF Driver1/KMDF Driver1.vcxproj
@@ -1,5 +1,8 @@
<?xml version="1.0" encoding="utf-8"?>
<Project DefaultTargets="Build" ToolsVersion="12.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+ <Import Project="..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props')" />
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props')" />
<ItemGroup Label="ProjectConfigurations">
<ProjectConfiguration Include="Debug|x64">
<Configuration>Debug</Configuration>
@@ -19,6 +22,7 @@
</ProjectConfiguration>
</ItemGroup>
<ItemGroup>
+ <None Include="packages.config" />
<None Include="ReadMe.txt" />
</ItemGroup>
<ItemGroup>
@@ -147,5 +151,15 @@
</ItemGroup>
<Import Project="$(VCTargetsPath)\Microsoft.Cpp.targets" />
<ImportGroup Label="ExtensionTargets">
+ <Import Project="..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets" Condition="Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets')" />
</ImportGroup>
+ <Target Name="EnsureNuGetPackageBuildImports" BeforeTargets="PrepareForBuild">
+ <PropertyGroup>
+ <ErrorText>This project references NuGet package(s) that are missing on this computer. Use NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}.</ErrorText>
+ </PropertyGroup>
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.props'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.targets'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.SDK.CPP.arm64.10.0.26100.2454\build\native\Microsoft.Windows.SDK.cpp.arm64.props'))" />
+ <Error Condition="!Exists('..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props')" Text="$([System.String]::Format('$(ErrorText)', '..\packages\Microsoft.Windows.WDK.ARM64.10.0.26100.2454\build\native\Microsoft.Windows.WDK.arm64.props'))" />
+ </Target>
</Project> The C driver samples repo also does something similar, albeit via director props override: I think maybe the best path forward here is for the nuget path to leverage the same env vars that the eWDK uses. It would be good if in the longterm, the nuget path better handles rust drivers, but for now we could provide instructions for setting the env var paths correctly. Ie. use this code path:
|
Enables the CI pipeline to use NuGet WDK/SDK packages instead of WinGet WDK installation.
These changes allow the workflows to use multiple WDK versions in the matrix.
NOTE: The latest WDK version is installed as a NuGet package. Older versions of WDK (before v10.0.26100) still rely on WinGet. On a local computer eWDK environement can be used.
Closes #214 partially. NI WDK and LLVM still use WinGet at the moment.
Summary of changes,
Modified workflows to use the above script to install WDK build numbers >= 26100.
Added packages.config that contains WDK packages and dependencies to be downloaded.