Skip to content

Conversation

@TheButlah
Copy link

@TheButlah TheButlah commented Jan 9, 2026

Closes #267

@TheButlah TheButlah force-pushed the thebutlah/allow-static-linkage branch from 43dbe40 to 1f2aa76 Compare January 9, 2026 06:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR enables optional static linkage for the TEEC library through a TEEC_STATIC environment variable. Previously, the code only supported dynamic linkage (dylib). The changes also include a typo fix in the function name from is_feature_enable to is_feature_enabled.

Key changes:

  • Adds support for static TEEC linkage controlled by the TEEC_STATIC environment variable
  • Refactors environment variable checking logic into a reusable is_env_present helper function
  • Corrects function name from is_feature_enable to is_feature_enabled

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

let feature_env = format!("CARGO_FEATURE_{}", feature.to_uppercase().replace("-", "_"));
is_env_present(&feature_env)
}

Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
/// 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.

Copilot uses AI. Check for mistakes.
}
Ok(())
}

Copy link

Copilot AI Jan 9, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
/// 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`.

Copilot uses AI. Check for mistakes.
@DemesneGH
Copy link
Contributor

Thanks for the PR!
Introducing an option for the linking method is a good improvement and the code looks good to me.

However, since this introduces a new implicit environment variable, could you help add some documentation in https://github.com/apache/teaclave-trustzone-sdk/blob/main/docs/advanced-setup.md?
A new section like "## Linkage Method to OP-TEE C Libraries" would be good to explain how to use TEEC_STATIC to control the linking method.

@ivila
Copy link
Contributor

ivila commented Jan 9, 2026

Thanks for the PR! Introducing an option for the linking method is a good improvement and the code looks good to me.

However, since this introduces a new implicit environment variable, could you help add some documentation in https://github.com/apache/teaclave-trustzone-sdk/blob/main/docs/advanced-setup.md? A new section like "## Linkage Method to OP-TEE C Libraries" would be good to explain how to use TEEC_STATIC to control the linking method.

I’d suggest keeping this simpler by adding a feature flag, as many other crates do—for example, the bundled feature in sqlite3-src

PS: I just learned that we can use the cfg! macro in build.rs to detect enabled features.
https://github.com/stainless-steel/sqlite3-src/blob/f92c0a8fac1b73b2de31987badc8aa227cddc11c/build.rs#L4-L8
image

@DemesneGH
Copy link
Contributor

I’d suggest keeping this simpler by adding a feature flag, as many other crates do—for example, the bundled feature in sqlite3-src

Since there‘s already no_link feature defined in the crate, adding a new feature for linkage method works for me.

@TheButlah
Copy link
Author

TheButlah commented Jan 9, 2026

Using feature flags in this manner exposes consumers of the code to dependency hell. If A consumes teec and B consumes A, I have to add a dependency from B -> teec as well (to define the feature flag), or I must re-export the feature flags in A.

This is brittle - if teec changes (which it can, even if the semver version of A doesn't), B now has an out-of-date dependency (version 0.7 of teec, but A has 0.8). Due to the links=teec setting in the cargo.toml of optee-teec-sys, B now gets an error.

Furthermore, if any crate in the dependency tree accidentally (or intentionally) enables the feature, its impossible to turn it off.

IMO its just tooling pain that a build-time env var is better suited to solve. And pkg-config defines a precedent and pattern for that.

@ivila ivila added the enhancement New feature or request label Jan 9, 2026
@ivila
Copy link
Contributor

ivila commented Jan 12, 2026

Using feature flags in this manner exposes consumers of the code to dependency hell. If A consumes teec and B consumes A, I have to add a dependency from B -> teec as well (to define the feature flag), or I must re-export the feature flags in A.

This is brittle - if teec changes (which it can, even if the semver version of A doesn't), B now has an out-of-date dependency (version 0.7 of teec, but A has 0.8). Due to the links=teec setting in the cargo.toml of optee-teec-sys, B now gets an error.

Furthermore, if any crate in the dependency tree accidentally (or intentionally) enables the feature, its impossible to turn it off.

IMO its just tooling pain that a build-time env var is better suited to solve. And pkg-config defines a precedent and pattern for that.

I see your point. In your setup, the crate that depends on optee-teec isn’t a client application but an invoker crate, so this does become awkward.
In that case, let’s keep the current approach.

if !is_feature_enable("no_link")? {
link();
if !is_feature_enabled("no_link")? {
link(is_env_present("TEEC_STATIC")?);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should check that it’s non-zero rather than just checking for presence, since it could be set as TEEC_STATIC=0.

For example:

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support statically linking libteec

3 participants