-
Notifications
You must be signed in to change notification settings - Fork 70
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: embed version and package metadata in binary #243
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
@CHMANG please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
Signed-off-by: CHMANG <[email protected]>
serde = "1.0" | ||
serde_json = "1.0" |
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 should be workspace dependencies
crates/wdk-build/src/lib.rs
Outdated
pub fn get_rc_root_path(&self) -> Result<String, ConfigError> { | ||
let bin_directory = self.wdk_content_root.join("bin"); | ||
|
||
// Add Windows SDK library paths based on logic from WindowsDriver.KernelMode.props & |
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 comment is copy-pasted. please clean up so that it makes sense
/// | ||
/// This function will return an error if any of the required paths do not | ||
/// exist. | ||
pub fn get_rc_root_path(&self) -> Result<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 looks like the same logic as in: https://github.com/CHMANG/windows-drivers-rs/blob/37f74cae7207571347706806121088917f5040ba/crates/wdk-build/src/cargo_make.rs#L516-L517
Can this logic be commonized to a single place in utils
as a free function that takes in a wdk content root path? that way it can be called in both setup_path
and in this function.
also can this Config method be named get_bin_path
since all the arch-specific bins are here.
let km_include_path = windows_sdk_include_path.join("km"); | ||
if !km_include_path.is_dir() { | ||
return Err(ConfigError::DirectoryNotFound { | ||
directory: crt_include_path.to_string_lossy().into(), | ||
directory: km_include_path.to_string_lossy().into(), | ||
}); | ||
} | ||
include_paths.push( | ||
crt_include_path | ||
km_include_path | ||
.canonicalize()? | ||
.strip_extended_length_path_prefix()?, | ||
); | ||
|
||
let km_or_um_include_path = windows_sdk_include_path.join(match self.driver_config { | ||
DriverConfig::Wdm | DriverConfig::Kmdf(_) => "km", | ||
DriverConfig::Umdf(_) => "um", | ||
}); | ||
if !km_or_um_include_path.is_dir() { | ||
let kit_shared_include_path = windows_sdk_include_path.join("shared"); | ||
if !kit_shared_include_path.is_dir() { | ||
return Err(ConfigError::DirectoryNotFound { | ||
directory: km_or_um_include_path.to_string_lossy().into(), | ||
directory: kit_shared_include_path.to_string_lossy().into(), | ||
}); | ||
} | ||
include_paths.push( | ||
km_or_um_include_path | ||
kit_shared_include_path | ||
.canonicalize()? | ||
.strip_extended_length_path_prefix()?, | ||
); | ||
|
||
let kit_shared_include_path = windows_sdk_include_path.join("shared"); | ||
if !kit_shared_include_path.is_dir() { | ||
let um_include_path = windows_sdk_include_path.join("um"); | ||
if !um_include_path.is_dir() { | ||
return Err(ConfigError::DirectoryNotFound { | ||
directory: kit_shared_include_path.to_string_lossy().into(), | ||
directory: um_include_path.to_string_lossy().into(), | ||
}); | ||
} | ||
include_paths.push( | ||
kit_shared_include_path | ||
um_include_path | ||
.canonicalize()? | ||
.strip_extended_length_path_prefix()?, | ||
); | ||
|
||
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 are you changing the include path logic? This changes how all the bindings are generated and break it.
The logic here strictly matches what happens in the vcxproj files of the WDK. If RC compilation requires different includes, it should be handled elsewhere
); | ||
} | ||
// function to get and set RC File with package metadata | ||
fn getandset_rcfile( |
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.
please follow rust conventions for function names
let mut productversion = String::new(); | ||
let mut name = String::new(); | ||
|
||
match fs::read_to_string("Cargo.toml") { |
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 is very hacky to get the crate version. You should use the env vars that cargo provides you: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-crates
original_filename = ver_originalfilename | ||
); | ||
|
||
std::fs::write("resources.rc", rc_content).expect("Unable to write RC file"); |
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.
error handling is needed in this module rather than panicking
fn get_packagemetadatadetails() -> (String, String, String) { | ||
// Run the 'cargo metadata' command and capture its output | ||
let path = env::var("CARGO_MANIFEST_DIR").unwrap(); | ||
let meta = MetadataCommand::new() |
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 do additional parsing here, do you think it would be better to bake this into the WDK config struct @ayodejiige ?
// Create the RC file content | ||
let rc_content = format!( | ||
r#"#include <windows.h> | ||
#include <ntverp.h> |
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.
Is there an MSDN doc that this template is pulled from? Are these all required? optional?
path::{Path, PathBuf}, | ||
process::Command, | ||
}; | ||
use cargo_metadata::MetadataCommand; |
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 file should probably be restructured such that it is some ResourceCompile builder, rather than a set of free functions. I think the process of compiling the rc file should be separated from the process of writing the rc file from a template to filesystem. This way, we could reuse it for different rc files, if needed
Includes automation capability to generate Resource files for both WDM and WDF driver development models by using cargo metadata.
Compile the Resource file using Resource compiler and link the compiled resource file into the application driver executable using the linker.