Closed cpu closed 1 year ago
cpu
commented
1 year ago 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
commented
1 year ago 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: https://github.com/bkchr/proc-macro-crate/pull/35
jsha
commented
1 year ago 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
commented
1 year ago 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.
:+1: 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
commented
1 year ago 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
commented
1 year ago 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.
jsha
commented
1 year ago 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
commented
1 year ago 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 :+1: Thanks!
jsha
commented
1 year ago 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
commented
1 year ago 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
commented
1 year ago @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
commented
1 year ago I wonder if we could do something like this?
Adopted! Thanks for the suggestion :snake: :rocket:
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. :+1:
And with that we're all green :green_circle: !
cpu
commented
1 year ago I think it would be worthwhile to file an upstream issue
:ballot_box_with_check: https://github.com/rust-lang/rust/issues/113209
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.