Copilot commented on code in PR #268:
URL:
https://github.com/apache/teaclave-trustzone-sdk/pull/268#discussion_r2675048498
##########
optee-teec/optee-teec-sys/build.rs:
##########
@@ -19,25 +19,29 @@ use std::env::{self, VarError};
use std::path::PathBuf;
fn main() -> Result<(), VarError> {
- if !is_feature_enable("no_link")? {
- link();
+ if !is_feature_enabled("no_link")? {
+ link(is_env_present("TEEC_STATIC")?);
}
Ok(())
}
Review Comment:
The `is_env_present` function is missing documentation. Consider adding a
doc comment to explain its purpose and behavior, similar to the
`is_feature_enabled` function below. This would improve code maintainability
and help other developers understand what this helper function does.
```suggestion
/// Checks whether the given environment variable is present.
/// Also emits a `cargo:rerun-if-env-changed` directive for Cargo build
scripts.
/// Returns `Ok(true)` if the variable is set, `Ok(false)` if it is not
present,
/// and propagates any other `VarError`.
```
##########
optee-teec/optee-teec-sys/build.rs:
##########
@@ -19,25 +19,29 @@ use std::env::{self, VarError};
use std::path::PathBuf;
fn main() -> Result<(), VarError> {
- if !is_feature_enable("no_link")? {
- link();
+ if !is_feature_enabled("no_link")? {
+ link(is_env_present("TEEC_STATIC")?);
}
Ok(())
}
-// Check if feature enabled.
-// Refer to:
https://doc.rust-lang.org/cargo/reference/features.html#build-scripts
-fn is_feature_enable(feature: &str) -> Result<bool, VarError> {
- let feature_env = format!("CARGO_FEATURE_{}",
feature.to_uppercase().replace("-", "_"));
-
- match env::var(feature_env) {
+fn is_env_present(var: &str) -> Result<bool, VarError> {
+ println!("cargo:rerun-if-env-changed={var}");
+ match env::var(var) {
Err(VarError::NotPresent) => Ok(false),
Ok(_) => Ok(true),
Err(err) => Err(err),
}
}
-fn link() {
+/// Checks if feature is enabled.
+/// Refer to:
https://doc.rust-lang.org/cargo/reference/features.html#build-scripts
+fn is_feature_enabled(feature: &str) -> Result<bool, VarError> {
+ let feature_env = format!("CARGO_FEATURE_{}",
feature.to_uppercase().replace("-", "_"));
+ is_env_present(&feature_env)
+}
+
Review Comment:
The `link` function now accepts a `static_linkage` parameter but lacks
documentation explaining this new parameter. Consider adding a doc comment or
updating existing documentation to explain when static vs dynamic linkage
should be used and what the parameter controls.
```suggestion
/// Configure Cargo to link against the OP-TEE client (`libteec`) library.
///
/// The `static_linkage` parameter controls whether `libteec` is linked
/// statically or dynamically:
/// * `true` – link `libteec` statically
(`cargo:rustc-link-lib=static=teec`),
/// which requires a static `libteec.a` to be available in
/// `$OPTEE_CLIENT_EXPORT/usr/lib`.
/// * `false` – link `libteec` dynamically
(`cargo:rustc-link-lib=dylib=teec`),
/// using the shared library provided by the system.
///
/// The value passed to `static_linkage` is derived from the `TEEC_STATIC`
/// environment variable in `main()`. Set `TEEC_STATIC` when you want the
/// build to use static linkage; leave it unset to use dynamic linkage.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
