Misc CI fixes. #322
Misc CI fixes. #322
Conversation
The existing CI has a step where it regenerates `rustls.h` using `make src/rustls.h`, which in turn invokes `cbindgen`. I'm seeing a diff created in CI that I don't see locally and suspect the reason is a difference in `cbindgen` versions. To help debug this now and into the future, this commit updates CI to print its `cbindgen` version before performing the diff check.
CI is using the latest available cbindgen (0.24.5), which produces a different formatted output than previous versions. This commit regenerates the `src/rustls.h` header file using 0.24.5 to match what CI expects, avoiding a CI failure from the detected diff.
Previously, building the tip of `main` in CI for Windows, Windows CMake (Debug), and Windows CMake (Release) was failing with link-time errors of the form: ``` rustls_ffi.lib(std-391022a4250a8b9a.std.feb3b897-cgu.0.rcgu.o) : error LNK2019: unresolved external symbol __imp_NtWriteFile referenced in function _ZN3std3sys7windows6handle6Handle17synchronous_write17h5e143db420a86fa8E [D:\a\rustls-ffi\rustls-ffi\build\tests\server.vcxproj] ``` The fix is to explicitly include `ntdll.lib` in the native static libs that we link on Windows. Doing this fixes the builds once we also update the `verify-static-libraries.py` script to expect this additional lib. This may be related to an upstream `rust-lang/rust` change[0] but I'm not 100% sure. [0]: rust-lang/rust#108262
This commit does two things: * In preparation for one day enabling merge queue on this repo, it adds `merge_group` as a trigger for the CI tasks. * It adds a `schedule` trigger that will run the CI tasks weekly, to detect bitrot sooner than we might otherwise.
It looks like the required checks in the repo configuration's branch protection rules are out-of-sync with the current checks being run. We'll need to fix that separately. |
Related discussion: bkchr/proc-macro-crate#35 |
We may just want to drop the num_enum / num_enum_derive dependency and do it ourselves. We need it in only two places, both of which are converting an input c_uint into the internal rustls_result enum. We could replace that with a big match, manually maintained, or with our own proc macro. |
👍 That's the same conclusion I reached w/ @djc in a Discord thread. I have a WIP commit replacing it with a |
I've removed this from the "Status checks that are required" section in branch protections. I haven't yet added the 1.60 variant, which I can do after this lands. CI still seems to think it's waiting on that one; perhaps a re-push will clear that up? Probably a better solution would be to tweak the job naming so instead of putting e.g. "1.57.0" or "1.60.0" in the job name, it says "MSRV." Then we wouldn't have to tweak the required status checks with every bump in MSRV. |
|
Pausing for the day, only one build task remains red now, from the static library test in the Windows CMake, Debug configuration listing lots of duplicates and not matching expected. got unexpected list of native static libraries, fix or update README. |
|
Very nice. I'm glad to be able to get rid of a dependency. The old I was surprised to see you didn't need to update the |
Aha, that answers the question I just left myself 👍 Thanks! |
|
BTW here's the ref for that: https://doc.rust-lang.org/std/convert/trait.TryFrom.html#implementors (can't link straight to the specific sub-hed) impl<T, U> TryFrom<U> for T
where
U: Into<T>,And https://doc.rust-lang.org/std/convert/trait.Into.html#implementors impl<T, U> Into<U> for T
where
U: From<T>,The list of static libraries is... weird. The feature of rustc that emits that list is documented, so in theory it is fully supported, but it seems to change in surprising ways between Rust versions. I think it would be worthwhile to file an upstream issue asking whether its output is expected to be stable, and whether additions to the list are monitored and considered meaningful. |
I wonder if we could do something like this? (aiui, ordering and repetitions in the list are valid and meaningful, but consecutive repetitions of the same library are not. even with this in place to elide the large numbers of |
|
@ctz I had started down a similar road locally but I like your approach better. I'll implement that and then see about opening an issue upstream to get clarity on what's up with the repetition. |
The `num_enum` dependency brings with it a large number of transitive dependencies. Some of those transitive deps now have an aggressive MSRV of 1.64+. The existing usage of `num_enum` is very minimal: just one derived trait for the `rustls_result` enum to provide it with a `From` impl for u32 primitive values. This commit replaces the `num_enum` crate with a small `u32_enum_builder!` macro rule loosely based on the Rustls `enum_builder` macro. Since our use case is narrower, I've simplified the macro and tailored it to the rustls-ffi use-case. With the new implementation we can also drop the use of `try_from`. In each case where we're converting from code -> result we're happy for the default `from` impl's `InvalidParameter` variant to be used when given an unknown code, making the use of `try_from` unnecessary. This approach adds one complication related to `cbindgen`: it now has to be instructed to expand the `rustls-ffi` crate before generating bindings in order to find the `rustls_result` enum. Doing this requires tweaking the `cbindgen.toml` to add a `parse.expand` configuration setting. Notably this also means `cbindgen` now has to be run with a nightly `rustc`. Since this is a developer only workflow it shouldn't be too onerous a requirement. We're now happily building with Rust 1.60 again and can also breathe easy knowing we have a slimmer dependency profile! tidy: replace rustls_result::try_from -> from.
In some instances (e.g. building the CMake configuration in debug mode with the nightly rustc) the `--print native-static-libs` output of `cargo build` can include many repeating instances of a lib. While the order and duplicates are generally expected to be meaningful, many consecutive duplicates are not. This commit updates the `verify-static-libraries.py` script to collapse consecutive duplicates before doing the check of expected vs actual.
Adopted! Thanks for the suggestion 🐍 🚀
I updated the commit that replaces And with that we're all green 🟢 ! |
|
Unfortunately the
mainbranch's CI has bitrot. This branch pulls together a handful of fixes.ci: print bindgen version used for rustls.h diff
The existing CI has a step where it regenerates
rustls.husingmake src/rustls.h, which in turn invokescbindgen.I'm seeing a diff created in CI that I don't see locally and suspect the reason is a difference in
cbindgenversions. To help debug this now and into the future, this commit updates CI to print itscbindgenversion before performing the diff check.src/rustls.h: regen with cbindgen 0.24.5
CI is using the latest available cbindgen (0.24.5), which produces a different formatted output than previous versions.
This commit regenerates the
src/rustls.hheader file using 0.24.5 to match what CI expects, avoiding a CI failure from the detected diff.makefiles: fix Windows unresolved symbol link errs.
Previously, building the tip of
mainin CI for Windows, Windows CMake (Debug), and Windows CMake (Release) was failing with link-time errors of the form:The fix is to explicitly include
ntdll.libin the native static libs that we link on Windows. Doing this fixes the builds once we also update theverify-static-libraries.pyscript to expect this additional lib.This may be related to an upstream
rust-lang/rustchange but I'm not 100% sure.cargo: track rustls MSRV, 1.57 -> 1.60
Rustls has updated its MSRV from 1.57 to 1.60. This commit tracks that change in this repo.
ci: cron schedule for workflows, merge_group support.
This commit does two things:
merge_groupas a trigger for the CI tasks.scheduletrigger that will run the CI tasks weekly, to detect bitrot sooner than we might otherwise.error: replace num_enum with minimal macro rule.
The
num_enumdependency brings with it a large number of transitive dependencies. Some of those transitive deps now have an aggressive MSRV of 1.64+.The existing usage of
num_enumis very minimal: just one derived trait for therustls_resultenum to provide it with aFromimpl for u32 primitive values.This commit replaces the
num_enumcrate with a smallu32_enum_builder!macro rule loosely based on the Rustlsenum_buildermacro. Since our use case is narrower, I've simplified the macro and tailored it to the rustls-ffi use-case.With the new implementation we can also drop the use of
try_from. In each case where we're converting from code -> result we're happy for the defaultfromimpl'sInvalidParametervariant to be used when given an unknown code, making the use oftry_fromunnecessary.This approach adds one complication related to
cbindgen: it now has to be instructed to expand therustls-fficrate before generating bindings in order to find therustls_resultenum. Doing this requires tweaking thecbindgen.tomlto add aparse.expandconfiguration setting. Notably this also meanscbindgennow has to be run with a nightlyrustc. Since this is a developer only workflow it shouldn't be too onerous a requirement.We're now happily building with Rust 1.60 again and can also breathe easy knowing we have a slimmer dependency profile!
tests: ignore consecutive duplicates in lib check.
In some instances (e.g. building the CMake configuration in debug mode with the nightly rustc) the
--print native-static-libsoutput ofcargo buildcan include many repeating instances of a lib.While the order and duplicates are generally expected to be meaningful, many consecutive duplicates are not.
This commit updates the
verify-static-libraries.pyscript to collapse consecutive duplicates before doing the check of expected vs actual.