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

fix nim CI; fix local testament #14102

Merged
merged 5 commits into from
Apr 24, 2020
Merged

Conversation

timotheecour
Copy link
Member

@timotheecour timotheecour commented Apr 24, 2020

  • fix running testament locally on OSX, doAssert execShellCmd("brew install gtk+3") == 0 would fail if that package was already installed
  • fix nim CI for important_packages.inim pending inim breaks nim CI inim-repl/INim#74
  • fix nim CI for thttpclient_ssl.nim (misc CI flaky tests timotheecour/Nim#113 (comment));
    on OSX the SSL error is printed to stderr and there's no way to capture it:
    123145372754092:error:14035418:SSL routines:ACCEPT_SR_CERT:tlsv1 alert unknown ca:/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-47.11.1/libressl-2.5/ssl/ssl_pkt.c:1205:SSL alert number 48

none of these worked:

proc ERR_lib_error_string*(e: cint): cstring{.cdecl,dynlib: DLLUtilName, importc.}
proc ERR_func_error_string*(e: cint): cstring{.cdecl,dynlib: DLLUtilName, importc.}
proc ERR_reason_error_string*(e: cint): cstring{.cdecl,dynlib: DLLUtilName, importc.}
proc ERR_error_string_n*(e: cint, buf: ptr char, len: csize_t){.cdecl,dynlib: DLLUtilName, importc.}

but checking for routines:CONNECT_CR_CERT:certificate verify failed seems correct according to https://mta.openssl.org/pipermail/openssl-users/2017-December/007046.html /cc @FedericoCeratto

SSL alert number 48 is specified in the documents that define SSL/TLS. It is the code for "unknown_ca", which means that verification failed

note

remaining CI failures (Nim SSL CI) are pre-existing ; these pipelines only run when affected code is modified but the failure is unrelated

check(msg.contains("shutdown while in init") or msg.contains("alert number 48"))
if not (msg.contains("shutdown while in init") or msg.contains("alert number 48") or
msg.contains("routines:CONNECT_CR_CERT:certificate verify failed")):
# pending https://github.com/nim-lang/Nim/pull/10558 simplify as: check(condition, msg)
Copy link
Member

Choose a reason for hiding this comment

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

Why this comment? #10558 has been rejected.

Copy link
Member Author

@timotheecour timotheecour Apr 24, 2020

Choose a reason for hiding this comment

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

I intend to re-open and rebase this PR in the near future, I keep running into cases that calls for this feature, which is available with doAssert cond, msg

The rationale that was given for rejection Tests and suites already have names. does not take this kind of situation into account, where the context (a runtime value!) only makes sense to show in the failure msg. As you can see in the above code, that context can't be shown in test suite name (and it wouldn't make sense either). We also don't want to dump context unconditionally, but only on failure, which is exactly what #10558 was implementing.

When this feature will be available, you'll be able to write the natural check(condition, msg), and you'll know why a test failed without having to modify it and run it locally, saving time.

Copy link
Member

Choose a reason for hiding this comment

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

I intend to re-open and rebase this PR in the near future

Ok, but this PR (or Nim codebase in general) shouldn't be your personal to-do list.

@narimiran narimiran merged commit d5b7e99 into nim-lang:devel Apr 24, 2020
@timotheecour timotheecour deleted the pr_fix_ci_ssl branch April 24, 2020 09:12
narimiran pushed a commit that referenced this pull request Apr 25, 2020
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.

2 participants