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

Update BthEchoSampleCli and BthEchoSampleSrv to be single architectur… #825

Merged
merged 3 commits into from
Jan 13, 2023

Conversation

dnl-q
Copy link
Contributor

@dnl-q dnl-q commented Jan 10, 2023

…e targeted and the specific build architecture is stamped at the time it is built.

…e targeted and the specific build architecture is stamped at the time it is built.
@jasonknichel
Copy link
Collaborator

The TargetOSVersion for the Models sections for both INFs previously was NT...1 which scopes this to a ProductType of VER_NT_WORKSTATION (see https://learn.microsoft.com/en-us/windows-hardware/drivers/install/inf-manufacturer-section ). You removed those ...1 so now the INFs will apply to all product types. Is that intentional? If you really do support all product types (which drivers usually do), we do support removing product type filtering, but I want to make sure that is intentional.

@jasonknichel
Copy link
Collaborator

The INFs have:

[BthEchoSampleCli_Inst.NT]
CopyFiles=Drivers_Dir

[Drivers_Dir]
BthEchoSampleCli.sys

and

[BthEchoSampleSrv_Inst.NT]
CopyFiles=Drivers_Dir

[Drivers_Dir]
BthEchoSampleSrv.sys

Which aren't specifically called out in the DestinationDirs section so they will use the DefaultDestDir which you updated to DIRID 13 (thanks!). But that means that these files are no longer copied to the "drivers directory", so the CopyFiles section name of "Drivers_Dir" is misleading. We should probably rename that section.

@jasonknichel
Copy link
Collaborator

You changed the DestinationDirs of CopyFiles section [BthEchoSampleCli_Inst_CoInstaller_CopyFiles], but you didn't change the AddReg registering the file copied by the CopyFiles section:

[BthEchoSampleCli_Inst_CoInstaller_AddReg]
HKR,,CoInstallers32,0x00010000, "WdfCoInstaller$KMDFCOINSTALLERVERSION$.dll,WdfCoInstaller"

However, CoInstallers are (long) deprecated and this WDF coinstaller hasn't been needed in the INF since around Windows 8 and we ignore it (hence why no InfVerif error about it), so you should be able to remove the WDF coinstaller references from your INF.

(Same comments apply to the similar sections/changes in the other INF)

Copy link
Collaborator

@jasonknichel jasonknichel left a comment

Choose a reason for hiding this comment

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

See comments

Rename CopyFiles section from Drivers_Dir to DriverStore_Dir.
Revert TargetOSVersion for the Models sections to NT$ARCH$...1, the same as before without my changes.
@dnl-q
Copy link
Contributor Author

dnl-q commented Jan 12, 2023

You changed the DestinationDirs of CopyFiles section [BthEchoSampleCli_Inst_CoInstaller_CopyFiles], but you didn't change the AddReg registering the file copied by the CopyFiles section:

[BthEchoSampleCli_Inst_CoInstaller_AddReg]
HKR,,CoInstallers32,0x00010000, "WdfCoInstaller$KMDFCOINSTALLERVERSION$.dll,WdfCoInstaller"

However, CoInstallers are (long) deprecated and this WDF coinstaller hasn't been needed in the INF since around Windows 8 and we ignore it (hence why no InfVerif error about it), so you should be able to remove the WDF coinstaller references from your INF.

(Same comments apply to the similar sections/changes in the other INF)

Thanks, I removed coinstaller references from both INX files.

@dnl-q
Copy link
Contributor Author

dnl-q commented Jan 12, 2023

The INFs have:

[BthEchoSampleCli_Inst.NT]
CopyFiles=Drivers_Dir

[Drivers_Dir]
BthEchoSampleCli.sys

and

[BthEchoSampleSrv_Inst.NT]
CopyFiles=Drivers_Dir

[Drivers_Dir]
BthEchoSampleSrv.sys

Which aren't specifically called out in the DestinationDirs section so they will use the DefaultDestDir which you updated to DIRID 13 (thanks!). But that means that these files are no longer copied to the "drivers directory", so the CopyFiles section name of "Drivers_Dir" is misleading. We should probably rename that section.

I renamed Drivers_Dir to DriverStore_Dir. Thanks.

@dnl-q
Copy link
Contributor Author

dnl-q commented Jan 12, 2023

The TargetOSVersion for the Models sections for both INFs previously was NT...1 which scopes this to a ProductType of VER_NT_WORKSTATION (see https://learn.microsoft.com/en-us/windows-hardware/drivers/install/inf-manufacturer-section ). You removed those ...1 so now the INFs will apply to all product types. Is that intentional? If you really do support all product types (which drivers usually do), we do support removing product type filtering, but I want to make sure that is intentional.

Recert back to NT$ARCH$...1. Thanks.

@jasonknichel
Copy link
Collaborator

I see that when you removed the coinstaller references, you also removed:

[BthEchoSampleCli_Inst.NT.Wdf]
KmdfService =  BthEchoSampleCli, BthEchoSampleCli_Inst_wdfsect

[BthEchoSampleCli_Inst_wdfsect]
KmdfLibraryVersion = $KMDFVERSION$

and

[BthEchoSampleSrv_Inst.NT.Wdf]
KmdfService =  BthEchoSampleSrv, BthEchoSampleSrv_Inst_wdfsect

[BthEchoSampleSrv_Inst_wdfsect]
KmdfLibraryVersion = $KMDFVERSION$

Those are not tied to the coinstaller and I believe should still be there (the coinstaller would look at them but I believe the new logic that makes the coinstaller unneccessary also looks at those). Can you please add them back?

@jasonknichel
Copy link
Collaborator

https://github.com/microsoft/Windows-driver-samples/blob/develop/bluetooth/bthecho/README.md says:

Copy KMDF coinstaller (wdfcoinstallerMMmmm.dll, from redist\wdf\ ), BthEchoSampleSrv.Sys, BthEchoSampleSrv.inf and bthsrvinst.exe on a temporary directory on the target machine.

and

Copy KMDF coinstaller (wdfcoinstallerMMmmm.dll, from redist\wdf\), BthEchoSampleCli.Sys, BthEchoSampleCli.inf and bthecho.exe on a temporary directory on the target machine.

With the removal of the coinstaller reference from the INFs, I believe those steps now aren't needed, right? If so, can you please update the README.md?

@dnl-q
Copy link
Contributor Author

dnl-q commented Jan 12, 2023

https://github.com/microsoft/Windows-driver-samples/blob/develop/bluetooth/bthecho/README.md says:

Copy KMDF coinstaller (wdfcoinstallerMMmmm.dll, from redist\wdf\ ), BthEchoSampleSrv.Sys, BthEchoSampleSrv.inf and bthsrvinst.exe on a temporary directory on the target machine.

and

Copy KMDF coinstaller (wdfcoinstallerMMmmm.dll, from redist\wdf\), BthEchoSampleCli.Sys, BthEchoSampleCli.inf and bthecho.exe on a temporary directory on the target machine.

With the removal of the coinstaller reference from the INFs, I believe those steps now aren't needed, right? If so, can you please update the README.md?

Done, thanks.

@dnl-q
Copy link
Contributor Author

dnl-q commented Jan 12, 2023

I see that when you removed the coinstaller references, you also removed:

[BthEchoSampleCli_Inst.NT.Wdf]
KmdfService =  BthEchoSampleCli, BthEchoSampleCli_Inst_wdfsect

[BthEchoSampleCli_Inst_wdfsect]
KmdfLibraryVersion = $KMDFVERSION$

and

[BthEchoSampleSrv_Inst.NT.Wdf]
KmdfService =  BthEchoSampleSrv, BthEchoSampleSrv_Inst_wdfsect

[BthEchoSampleSrv_Inst_wdfsect]
KmdfLibraryVersion = $KMDFVERSION$

Those are not tied to the coinstaller and I believe should still be there (the coinstaller would look at them but I believe the new logic that makes the coinstaller unneccessary also looks at those). Can you please add them back?

Done, thanks.

@jasonknichel jasonknichel merged commit 8cfa11c into microsoft:develop Jan 13, 2023
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.

2 participants