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.

Sign up for GitHub

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

linking C libraries staticly and Bazel compatibility #371

Merged
merged 6 commits into from
Nov 11, 2024
Merged

linking C libraries staticly and Bazel compatibility #371

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c1554d6
removing linking native libs in platform-support crate
irajtaghlidi Sep 23, 2024
0001b6c
calculate include path from out_dir
irajtaghlidi Sep 23, 2024
3244473
set static linking
irajtaghlidi Sep 23, 2024
7e57229
Linking C static libs for mbedtls_printf
irajtaghlidi Sep 23, 2024
f96fefe
Windows compatibility
irajtaghlidi Sep 23, 2024
f43ba5d
deleting mbedtls_include variable
irajtaghlidi Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion mbedtls-sys/build/bindgen.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl super::BuildConfig {
if cc.get_compiler().is_like_msvc() {
cc.flag("--driver-mode=cl");
}
cc.include(&self.mbedtls_include).define(
cc.include(&self.out_dir.join("include")).define(
"MBEDTLS_CONFIG_FILE",
Some(format!(r#""{}""#, self.config_h.to_str().expect("config.h UTF-8 error")).as_str()),
);
Expand Down
3 changes: 0 additions & 3 deletions mbedtls-sys/build/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ use std::path::{Path, PathBuf};
struct BuildConfig {
out_dir: PathBuf,
mbedtls_src: PathBuf,
mbedtls_include: PathBuf,
config_h: PathBuf,
cflags: Vec<String>,
}
Expand Down Expand Up @@ -83,7 +82,6 @@ impl BuildConfig {
let out_dir = PathBuf::from(env::var_os("OUT_DIR").expect("OUT_DIR environment not set?"));
let config_h = out_dir.join("config.h");
let mbedtls_src = PathBuf::from(env::var("RUST_MBEDTLS_SYS_SOURCE").unwrap_or("vendor".to_owned()));
let mbedtls_include = mbedtls_src.join("include");

let mut cflags = vec![];
if FEATURES.have_platform_component("c_compiler", "freestanding") {
Expand All @@ -97,7 +95,6 @@ impl BuildConfig {
config_h,
out_dir,
mbedtls_src,
mbedtls_include,
cflags,
}
}
Expand Down
17 changes: 6 additions & 11 deletions mbedtls-sys/build/cmake.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,25 +46,20 @@ impl super::BuildConfig {
cmk.define("CMAKE_TRY_COMPILE_TARGET_TYPE", "STATIC_LIBRARY");
}

let mut dst = cmk.build();
let dst = cmk.build();

dst.push("lib");
jethrogb marked this conversation as resolved.
Show resolved Hide resolved
println!(
"cargo:rustc-link-search=native={}",
dst.to_str().expect("link-search UTF-8 error")
dst.join("lib").to_str().expect("link-search UTF-8 error")
);

println!("cargo:rustc-link-lib=mbedtls");
println!("cargo:rustc-link-lib=mbedx509");
println!("cargo:rustc-link-lib=mbedcrypto");
println!("cargo:rustc-link-lib=static=mbedtls");
println!("cargo:rustc-link-lib=static=mbedx509");
println!("cargo:rustc-link-lib=static=mbedcrypto");
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the affect of newly added static=?
Does it just explicitly ensure static link?

Copy link
Contributor Author

@irajtaghlidi irajtaghlidi Sep 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

What's the effect of newly added static=?
Does it just explicitly ensure static link?

It does not have an effect in Cargo but setting static linking explicitly makes Bazel's Rust rule work properly.

More Context:
Since they are built script's output in a lib (rlib) crate, it should and would linked statically to the final ELF and Cargo handles it automatically even if we don't mention it.
But Rust's rule in Bazel is trusting what we asked for. Since the default link value is dynamic, it tries to find and link these native static libraries (.a archive files) dynamically which fails.
link the generated crate to a native library

So it would be better for both to define explicitly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@irajtaghlidi why doesn't this change have an effect when building with Cargo?

On the page you linked above, I see

The kind of library and the modifiers can also be specified in a #[link] attribute. If the kind is not specified in the link attribute or on the command-line, it will link a dynamic library by default, except when building a static executable. If the kind is specified on the command-line, it will override the kind specified in a link attribute.

In the original code, the kind is not specified in the link attribute nor on the command line, so wouldn't cargo try to link a dynamic library?

I understand that the mbedtls-sys build script builds a static lib version of mbedtls and hence we want to link that static lib, but I haven't spotted how the build script tells Cargo that it's a static lib. The documentation (or at least my interpretation of it) seems to indicate that Cargo would try to link a dynamic lib by default, so I would expect the original code to fail. Probably I'm missing something...

Copy link
Member

Choose a reason for hiding this comment

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

My guess is Rust tries to link in whatever way it can, it finds that the only available option is static, and does that.

I also note that mbedtls-platform-support already does this:

println!("cargo:rustc-link-lib=static=mbedtls");
println!("cargo:rustc-link-lib=static=mbedx509");
println!("cargo:rustc-link-lib=static=mbedcrypto");

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, this specific change seems fine.


println!(
"cargo:include={}",
::std::env::current_dir()
.unwrap()
.join(&self.mbedtls_include)
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this var is used anywhere else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't. This part is just trying to provide header files location to dependent crates that can be read by DEP_MBEDTLS_INCLUDE environment variable. Either from the source directory or alternatives (out_dir). and from dependent crates, it does not matter as long as it has read access.
Cargo uses a flat model and you have full access to the file system but Bazel sandboxing requires some practices to follow for more compatibility.

Copy link
Member

Choose a reason for hiding this comment

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

So then the var can be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

self.mbedtls_include is still being used by Bindgen but I'll check if headers are also accessible in there, Since it's running after the CMake step.

fn main() {
    let cfg = BuildConfig::new();
    cfg.create_config_h();
    cfg.print_rerun_files();
    cfg.cmake();
    cfg.bindgen();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now, Only CMake depends on the source path and other components are using OUT_DIR content.

.to_str()
.expect("include/ UTF-8 error")
dst.join("include").to_str().expect("include/ UTF-8 error")
);
println!("cargo:config_h={}", self.config_h.to_str().expect("config.h UTF-8 error"));
}
Expand Down
Loading