Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1184 +/- ##
==========================================
- Coverage 95.80% 92.76% -3.04%
==========================================
Files 61 71 +10
Lines 8143 10334 +2191
Branches 0 10334 +10334
==========================================
+ Hits 7801 9586 +1785
- Misses 342 453 +111
- Partials 0 295 +295 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
glebpom
changed the title
The ("windows", _, Static) arm produced the same name as the catch-all
static arm, tripping clippy::match_same_arms under -W clippy::pedantic.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| assert!( | ||
| ssl_path.is_file(), | ||
| "AWS-LC libssl artifact not found: {}", | ||
| ssl_path.display() | ||
| ); |
There was a problem hiding this comment.
Since asserts fail the build for all consumers (not just ones reading these vars), could these be downgraded to cargo:warning?
| @@ -31,19 +31,37 @@ fn main() { | |||
| } | |||
|
|
|||
| fn build_and_link(links: &str, target_name: &str) { | |||
There was a problem hiding this comment.
Our CI job for "links-testing" should be extended to cover a few more build configurations:
- Windows dynamic -- both
*-windows-gnuand*-windows-mscv - ssl consumer coverage -- an ssl-enabled feature in
links-testingthat assertsDEP_*_LIBSSL_PATH
| fn library_filename(self, name: &str) -> String { | ||
| match (target_os().as_str(), target_env().as_str(), self) { | ||
| ("windows", "msvc", _) => format!("{name}.lib"), | ||
| ("windows", _, Self::Dynamic) => format!("lib{name}.dll.a"), |
There was a problem hiding this comment.
I'm not sure this is true for our prefixed source builds. The sys crates' CMakeLists.txt only sets IMPORT_PREFIX for MSVC; on MinGW the import library keeps CMake's default lib prefix, so we'd get libcrypto.dll.a while this predicts libaws_lc_X_Y_Z_crypto.dll.a.
- With the
assert!below, this might breakAWS_LC_SYS_STATIC=0builds on windows-gnu, which currently link fine because ld falls back to the DLL directly.
| println!("cargo:libcrypto_path={}", crypto_path.display()); | ||
| println!("cargo:link_kind={}", output_lib_type.rust_lib_type()); | ||
|
|
||
| if cfg!(feature = "ssl") { |
There was a problem hiding this comment.
We need to add an ssl-enabled feature to links-testing that asserts DEP_*_LIBSSL_PATH the same way as libcrypto, plus a matrix entry to the links-crate-tests job.
|
I've stacked five follow-up commits addressing my review feedback on top of your branch here: https://github.com/justsmth/aws-lc-rs/tree/pr-1184-review-fixes Since this PR comes from an organization-owned fork, I can't push to the branch directly (GitHub doesn't support maintainer edits for org forks). My changes cover:
You can bring them into this PR with: I'd also be fine with closing this PR and creating a separate one with these changes, if you'd prefer. NOTE: my changes grow the CI matrix and exercise some combinations for the first time (ssl on macOS/Windows, windows-gnu dynamic), so we may see new failures that I can help triage. |
|
@justsmth Thanks for the review! Since you've already fixed the remaining issues - please proceed with merging your PR, I'm totally fine with that |
Description of changes:
Currently, the sys crates export include directories and linker directives, but downstream Cargo build scripts cannot reliably determine the exact native library artifacts produced by AWS-LC. This is especially problematic when compiling additional C code against AWS-LC because source builds use version-prefixed library names.
This change exports additional build metadata from
aws-lc-sysandaws-lc-fips-sys:libdirlibcrypto_pathlibssl_pathwhen thesslfeature is enabledlink_kindThe metadata is emitted consistently for CC builds, CMake builds, and system-library builds. Platform-aware artifact naming is included for static and dynamic libraries on Unix, Apple platforms, MinGW, and MSVC.
This enables downstream build scripts to pass AWS-LC’s exact headers and libraries to native build systems such as CMake, without rebuilding AWS-LC or guessing version-prefixed artifact names.
Call-outs:
The new metadata becomes an interface consumed by direct Cargo dependents through versioned
DEP_AWS_LC_*andDEP_AWS_LC_FIPS_*environment variables.Exact absolute artifact paths are exported intentionally because the existing logical library names are insufficient for native build systems that require a concrete library file.
Special review attention would be helpful for:
link_kindreporting.libssl_pathwhen thesslfeature is enabled.Existing Rust linker directives and linking behavior are unchanged.
Testing:
The
links-testingcrate now verifies that:libcryptopath exists.staticordylib.The metadata was tested with:
aws-lc-sysusing the CC builder.aws-lc-sysusing the CMake builder.aws-lc-fips-sysusing its CMake build.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.