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.

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

Misc CI fixes. #322

Merged
merged 7 commits into from
Jun 30, 2023
Merged

Misc CI fixes. #322

merged 7 commits into from
Jun 30, 2023

Conversation

cpu
Copy link
Member

@cpu cpu commented Jun 29, 2023

Unfortunately the main branch'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.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.

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.h header 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 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 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:

  • 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.

error: replace num_enum with minimal macro rule.

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!

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-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.

cpu added 5 commits June 29, 2023 11:39
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.
@cpu
Copy link
Member Author

cpu commented Jun 29, 2023

Build+test (clang, 1.57.0, ubuntu-20.04) Expected — Waiting for status to be reported

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.

@cpu
Copy link
Member Author

cpu commented Jun 29, 2023

I haven't yet been able to figure out an arrangement that will build on Rust 1.60, even though the num_enum and num_enum_derive both advertise a MSRV of 1.57.

Related discussion: bkchr/proc-macro-crate#35

@jsha
Copy link
Collaborator

jsha commented Jun 29, 2023

Fix the MSRV errors from num_enum + num_enum_derive

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.

@cpu
Copy link
Member Author

cpu commented Jun 29, 2023

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 macro_rules! rule, but I'm presently stuck getting bindgen to be happy with it. I think it needs to be coerced into expanding the macro first.

@jsha
Copy link
Collaborator

jsha commented Jun 29, 2023

Build+test (clang, 1.57.0, ubuntu-20.04) Expected — Waiting for status to be reported

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.

@cpu
Copy link
Member Author

cpu commented Jun 29, 2023

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.
Got:
 advapi32.lib credui.lib kernel32.lib secur32.lib legacy_stdio_definitions.lib kernel32.lib advapi32.lib advapi32.lib bcrypt.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib kernel32.lib ntdll.lib ntdll.lib ntdll.lib ntdll.lib userenv.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib ws2_32.lib kernel32.lib ws2_32.lib kernel32.lib kernel32.lib msvcrt.lib
Instead of:
 advapi32.lib credui.lib kernel32.lib secur32.lib legacy_stdio_definitions.lib kernel32.lib advapi32.lib userenv.lib kernel32.lib kernel32.lib ws2_32.lib bcrypt.lib ntdll.lib msvcrt.lib legacy_stdio_definitions.lib

src/session.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
@jsha
Copy link
Collaborator

jsha commented Jun 29, 2023

Very nice. I'm glad to be able to get rid of a dependency.

The old TryFromPrimitive implemented TryFrom. Your new macro implements From, with a default of InvalidParameter. Looking at the call sites, that makes sense to me. We can accept a default rather than specifically handling an out-of-range value in all places.

I was surprised to see you didn't need to update the rustls_result::try_from call sites, until I realized that there's a blanket impl of TryFrom for all From. Still, it would be best to update those call sites to use ::from instead so they are not misleading. You'll notice that those call sites appear to do something special in the error case but I believe from looking at them that it's totally fine for them to just receive InvalidParameter instead; though of course go ahead and check my logic. :D

@cpu
Copy link
Member Author

cpu commented Jun 29, 2023

I was surprised to see you didn't need to update the rustls_result::try_from call sites, until I realized that there's a blanket impl of TryFrom for all From.

Aha, that answers the question I just left myself 👍 Thanks!

@jsha
Copy link
Collaborator

jsha commented Jun 29, 2023

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.

@ctz
Copy link
Member

ctz commented Jun 30, 2023

The list of static libraries is... weird.

I wonder if we could do something like this?

diff --git a/tests/verify-static-libraries.py b/tests/verify-static-libraries.py
index 8f82130..875b247 100755
--- a/tests/verify-static-libraries.py
+++ b/tests/verify-static-libraries.py
@@ -8,6 +8,15 @@ STATIC_LIBS_RE = re.compile(
     b"note: native-static-libs: ([^\\n]+)\\n"
 )
 
+def uniquify_consecutive(items):
+    r = []
+    for i in items.split():
+        if not (r and r[-1] == i):
+            r.append(i)
+    return r
+
 
 def main():
     # If you need to change the values here, be sure to update the values in
@@ -40,7 +49,7 @@ def main():
               "compilation errors")
         sys.exit(1)
     got = match.group(1).decode("ascii")
-    if want != got:
+    if uniquify_consecutive(want) != uniquify_consecutive(got):
         print(
             "got unexpected list of native static libraries, "
             "fix or update README. Got:\n {}\nInstead of:\n {}"

(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 kernel32.lib, the list of libraries has changed.)

@cpu
Copy link
Member Author

cpu commented Jun 30, 2023

@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.

cpu added 2 commits June 30, 2023 10:49
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.
@cpu
Copy link
Member Author

cpu commented Jun 30, 2023

I wonder if we could do something like this?

Adopted! Thanks for the suggestion 🐍 🚀

Still, it would be best to update those call sites to use ::from instead so they are not misleading. You'll notice that those call sites appear to do something special in the error case but I believe from looking at them that it's totally fine for them to just receive InvalidParameter instead; though of course go ahead and check my logic. :D

I updated the commit that replaces num_enum to also update the conversion sites to use from instead of try_from. I reached the same conclusion as you: InvalidParameter seems like a fine default in each case. 👍

And with that we're all green 🟢 !

@cpu cpu merged commit 2508f98 into rustls:main Jun 30, 2023
@cpu cpu deleted the cpu-ci-fixup branch June 30, 2023 15:16
@cpu
Copy link
Member Author

cpu commented Jun 30, 2023

I think it would be worthwhile to file an upstream issue

☑️ rust-lang/rust#113209

@cpu cpu mentioned this pull request Jul 14, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants