-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
RFC: Add basic support for automated WDK installation #111
Conversation
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, this looks very good, thanks!
Could we add some minimal testing of it in the CI setup? E.g. enable the --with-wdk-installers
in one setup, and then e.g. try compiling and linking some minimal test driver? (Doesn't need to be a full test driver from somewhere, just something minimal with a single entry point function or similar, would be enough, as long as it testruns including a kernel mode header and linking a kernel mode library.)
As you're adding support for it in msbuild, I presume that's your actual use case, so it might be good with a test for that as well (although I'm not sure if I'd consider it mandatory here), but it would be good with a separate plain test of just one straight cl.exe
invocation as well, to test whether files were installed correctly.
Not sure what the best way is to detect whether this actually was meant to be installed though. Maybe a flag like HAVE_WDK=1 ./test.sh
? Since we do add paths like km
to the INCLUDE path unconditionally, even if they weren't installed, we can't check if they're present in the INCLUDE variable. And if we check whether including such a header succeeds, the test just remains silent if we fail to install what we meant to.
SDK_INCDIR="kits/10/include/$SDKVER/$incdir" | ||
|
||
if [ -d "$SDK_INCDIR" ]; then | ||
$ORIG/fixinclude -map_winsdk "$SDK_INCDIR" |
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.
Can you merge these two loops? It would deviate from the order in the script originally, but I think it should be harmless (fixinclude doesn't look at files outside of the given directory).
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 was not sure whether there are dependencies between lowercase
and fixinclude
, and just assumed that all headers must be lowercased for fixinclude
to work. I'll merge them into one loop, then.
I thought about doing this, but it will add ~700 MB of downloads to the CI job and ~ 1.5 GB to the install size. As for the test driver, I could take a MIT licensed example driver project from Microsoft GitHub, just as I did with MSBuild tests. |
Right, it probably would make the CI job a bit slower. The install size should probably be fine, as we don't distribute the installed directory anywhere anyway (and aren't allowed to anyway). OTOH if it needlessly hammers Microsoft servers, where they aren't prepared to handle very frequent extra downloads, it might not be so nice...
Oh, I missed this detail.
That would certainly be appreciated indeed.
Sure, that would probably be good. |
a2fd5fd
to
822997f
Compare
I getting strange |
That sounds odd. Yes, it’s ok to not test it on all platforms, as long as it’s tested in at least one configuration in CI. That said, if you want to debug it, I would add lots of debug printouts in vsdownload.py, to show what files it sees and how it progrfilesesses. But it’s a bit surprising to hear about such failures in unpacking(?) files. I wonder if it’s due to a case insensitive file system, or if it is due to a different version of Python. |
Wait, does macOS have a case-insensitive file system? Isn't it a kind of Unix system since Mac OS X days? |
Yes, it is very much a Unix (even a certified one), but they have retained the case insensitiveness. It is possible to set the apple file systems to case sensitive mode too (although I think it might be kinda cumbersome as I’m not sure if it’s possible to change it other than when formatting a partition), but I’ve heard issues with e.g. some Adobe applications misbehaving on case sensitive file systems. For external volumes it’s certainly doable though. |
This is very unfortunate, because my |
822997f
to
65d275b
Compare
Add a new command line option '--with-wdk-installers' pointing to the directory where the MSI packages downloaded by wdksetup.exe are located. Extract the WDK packages after unpacking the Windows SDK packages. They both install files in the same versioned directories. Unpack the included VS extension files onto the MSVC tree. Fix the WDK directory structure after extracting MSI packages with msiextract on Linux.
Lowercase kernel mode libraries as well.
Set the required WDK version property and disable the automated driver signing and Universal API validation because it does not work in WINE.
Download wdksetup.exe with curl and run it in the automated setup mode in WINE.
Add a minimal test WDF driver project that is built with MSBuild. Run the WDK driver tests only when the `wine-mono` package is installed and the `HAVE_WDK` environment variable is defined.
ca931db
to
b209874
Compare
The WDK downloader is a 32-bit Windows executable.
Install WDK using the official online installer from the Microsoft website. Enable WDK tests in GitHub Actions for Linux only.
b209874
to
acb31a1
Compare
I've rebased on master and fixed the Ubuntu CI job. macOS tests are not enabled because the WDK installation does not work on case-insensitive file systems with the current implementation. Is it OK to leave this as a Linux-only feature? I don't know if there's any demand for it, since developing Windows drivers on macOS is not something people usually do. |
Yes, that's totally ok - if it's a less central feature, I'm quite happy with it being tested only in one configuration. As long as it is tested in at least one configuration, it won't be dead code that I don't know how to test. I'll try to get to re-reviewing this PR within a few days, hopefully! |
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.
Looking really good now, thanks! Just a couple discussion points left, otherwise ok!
@@ -109,6 +109,10 @@ for arch in x86 x64 arm arm64; do | |||
|| -d /usr/local/share/wine/mono \ | |||
|| -d /opt/wine/mono ]]; then | |||
EXEC "" BIN=$BIN ./test-msbuild.sh | |||
|
|||
if [[ -n $HAVE_WDK && ($arch == "x64" || $arch == "arm64") ]]; then |
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 presume this limitation on arch is based on what is supported in the vcxproj file?
Is it possible to do a minimal-ish command line compilation of the same as well? (Not required as part of this PR, but would be nice to have.)
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 x86_64
and aarch64
are the only architectures supported by current Windows kernels. There used to be 32-bit versions of Windows 10, but it's not something people care about in 2024.
As for the command line test, I wanted to add a plain Makefile, but it turned out it requires an enormous amount of compile and link flags even for the simplest drivers. And many of them are WDK version specific, as you can see here: https://github.com/mstorsjo/msvc-wine/actions/runs/7986251670/job/21806269948#step:5:568 Such a test would probably be very fragile wrt. WDK updates.
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.
Ok, fair enough, thanks for explaining!
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.
@ravenexp, all versions of Windows 10 support 32-bit architectures. It's only Windows 11 that doesn't.
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.
Oh, I didn't know that. I can add x86
back to the test matrix if your use case requires this target.
Add a new command line option '--with-wdk-installers' pointing to the directory where the MSI packages downloaded by
wdksetup.exe
are located.Extract the WDK packages after unpacking the Windows SDK packages.
They both install files in the same versioned directories.
Unpack the included VS extension files onto the MSVC tree.
Fix the WDK directory structure after extracting MSI packages with
msiextract
on Linux.Resolves #28