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

fix: skip infverif task for sample drivers built with certain GE WDK versions #143

Merged
merged 47 commits into from
May 31, 2024

Conversation

NateD-MSFT
Copy link
Contributor

@NateD-MSFT NateD-MSFT commented Apr 26, 2024

We expect that drivers built for Windows pass InfVerif. However, the driver samples provided by Microsoft generally have settings that do not pass the default InfVerif ruleset.

In the current (E)WDK, this can be alleviated by setting /msft when calling InfVerif.exe, but in the upcoming release's WDK this syntax is no longer available. While we wait for the InfVerif team to enable a new "/samples" flag for us, this change updates the cargo make logic to:

  • Gather the WDK version
  • Check the version against the build where InfVerif changed behavior
  • If using the old WDK, use "/msft"; otherwise, indicate that we would like to use "/samples" but in practice skip the InfVerif task for the moment.

Note that with this change, samples should extend the makefile with

[tasks.infverif]
dependencies = ["wdk-samples-setup"]
condition = { env_not_set = ["WDK_INF_USING_NEW_VERSION"] }

When testing on one machine, I did see an issue where a ; was incorrectly appended to the /msft flag. I haven't been able to repro this on other machines yet.

@wmmc88 wmmc88 requested review from wmmc88 and a team April 29, 2024 16:02
Copy link
Collaborator

@wmmc88 wmmc88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @NateD-MSFT! Thanks for the PR.

I like the change overall, but was wondering if we could isolate the sample-specific behavior outside of the core wdk-crates. This way we can avoid polluting windows-drivers-rs with extra environment that is not relevant to the wdk or to building rust drivers.

My suggestion:

  • keep the logic to detect, validate, and expose the WDK version inside of wdk-build and rust-driver-makefile.toml
  • move the sample-specific infverif version mitigation logic to a script block in sample-kmdf-driver/Makefile.toml (and the same makefiles in the samples repo). It can be refactored to update WDK_BUILD_ADDITIONAL_INFVERIF_FLAGS by printing to stdout and using the rust-env-update plugin I wrote.

crates/sample-kmdf-driver/Makefile.toml Outdated Show resolved Hide resolved
crates/wdk-build/rust-driver-makefile.toml Outdated Show resolved Hide resolved
crates/wdk-build/rust-driver-makefile.toml Outdated Show resolved Hide resolved
crates/wdk-build/src/cargo_make.rs Outdated Show resolved Hide resolved
crates/wdk-build/src/cargo_make.rs Outdated Show resolved Hide resolved
@wmmc88
Copy link
Collaborator

wmmc88 commented Apr 29, 2024

  • move the sample-specific infverif version mitigation logic to a script block in sample-kmdf-driver/Makefile.toml (and the same makefiles in the samples repo). It can be refactored to update WDK_BUILD_ADDITIONAL_INFVERIF_FLAGS by printing to stdout and using the rust-env-update plugin I wrote.

On 2nd thought, given that this is to work around issues in production WDKs and will be needed in all msft samples, I think we can keep a public function in wdk_build::cargo_make like setup_wdk_samples_infverif that contains workarounds like this. I still would like to keep it outside of wdk-build-init that every driver build goes thru. One approach is creating a new wdk-samples-setup task in rust-driver-makefile.toml and overriding the dependencies of infverif task to add the wdk-samples-setup in the samples (both in this repo and in sample repo). The wdk-samples-setup would call setup_wdk_samples_infverif and would update the executing task using the rust-env-update plugin

@NateD-MSFT NateD-MSFT requested a review from wmmc88 May 1, 2024 17:20
@wmmc88 wmmc88 self-assigned this May 2, 2024
@NateD-MSFT
Copy link
Contributor Author

I did one more quick refactor to clean things up, go ahead and review when you have time.

NateD-MSFT and others added 10 commits May 13, 2024 15:48
Committing and signing off to see if the cloud build works where my local environment doesn't.

Co-authored-by: Melvin Wang <melvin.mc.wang@gmail.com>
Signed-off-by: NateD-MSFT <34494373+NateD-MSFT@users.noreply.github.com>
I thought this needed to be made pub when I was testing, but I think it was a side effect of how I was testing my script while blocked on the cargo make bug.  So this shouldn't need to be pub specifically.
@NateD-MSFT
Copy link
Contributor Author

Could you also spin up an accompanying pr for the samples repo? It won't merge until we do another crates.io release for WDR, but you can test the changes using git or path deps in cargo

Created here: microsoft/Windows-rust-driver-samples#18

@wmmc88 wmmc88 changed the title Add logic to handle driver samples differently for InfVerif fix: skip infverif task for sample drivers built with certain GE WDK versions May 30, 2024
@wmmc88 wmmc88 added this pull request to the merge queue May 31, 2024
Merged via the queue into microsoft:main with commit 9a02d5b May 31, 2024
49 checks passed
@wmmc88 wmmc88 mentioned this pull request Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants