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

Add MbedTLS as a stdlib #32706

Closed
wants to merge 5 commits into from
Closed

Add MbedTLS as a stdlib #32706

wants to merge 5 commits into from

Conversation

quinnj
Copy link
Member

@quinnj quinnj commented Jul 26, 2019

No description provided.

@KristofferC
Copy link
Member

KristofferC commented Jul 26, 2019

None of the MbedTLS files should actually be commited to this repo. They get pulled in during building from a repo at a certain commit. Look at how Pkg does it:

julia/stdlib/Makefile

Lines 21 to 23 in 16fdcf7

STDLIBS_EXT = Pkg
PKG_GIT_URL := git://github.com/JuliaLang/Pkg.jl.git
PKG_TAR_URL = https://api.github.com/repos/JuliaLang/Pkg.jl/tarball/$1

and

https://github.com/JuliaLang/julia/blob/master/stdlib/Pkg.version

@quinnj quinnj force-pushed the jq/mbedtls branch 2 times, most recently from e06ecba to e2ccb60 Compare August 6, 2019 13:42
@quinnj quinnj closed this Aug 6, 2019
@quinnj quinnj reopened this Aug 6, 2019
@quinnj
Copy link
Member Author

quinnj commented Aug 6, 2019

Hmmmm, anyone know what this error is?

Mmap  ───────────  0.146358 seconds
Serialization  ──  2.594393 seconds
Libdl  ──────────  0.092973 seconds
Markdown  ───────  2.888116 seconds
MbedTLS  ──────── 10.474995 seconds
error during bootstrap:
LoadError("sysimg.jl", 16, LoadError("/Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.3/LibGit2/src/LibGit2.jl", 21, LoadError("/Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.3/LibGit2/src/utils.jl", 44, ErrorException("error compiling version: could not load library \"libgit2\"\ndlopen(libgit2.dylib, 1): Library not loaded: @rpath/libmbedcrypto.3.dylib\n  Referenced from: /Users/julia/buildbot/worker/package_macos64/build/usr/lib/libgit2.dylib\n  Reason: image not found"))))
rec_backtrace at /Users/julia/buildbot/worker/package_macos64/build/src/stackwalk.c:94
record_backtrace at /Users/julia/buildbot/worker/package_macos64/build/src/task.c:224
jl_throw at /Users/julia/buildbot/worker/package_macos64/build/src/task.c:463
jl_rethrow_with_add at /Users/julia/buildbot/worker/package_macos64/build/src/codegen.cpp:791
jl_compile_linfo at /Users/julia/buildbot/worker/package_macos64/build/src/codegen.cpp:1201
jl_compile_method_internal at /Users/julia/buildbot/worker/package_macos64/build/src/gf.c:1778
_jl_invoke at /Users/julia/buildbot/worker/package_macos64/build/src/gf.c:2048 [inlined]
jl_apply_generic at /Users/julia/buildbot/worker/package_macos64/build/src/gf.c:2213
jl_apply at /Users/julia/buildbot/worker/package_macos64/build/src/./julia.h:1630 [inlined]
do_call at /Users/julia/buildbot/worker/package_macos64/build/src/interpreter.c:328
eval_stmt_value at /Users/julia/buildbot/worker/package_macos64/build/src/interpreter.c:368 [inlined]
eval_body at /Users/julia/buildbot/worker/package_macos64/build/src/interpreter.c:764
jl_interpret_toplevel_thunk_callback at /Users/julia/buildbot/worker/package_macos64/build/src/interpreter.c:888
Interpreter frame (ip: 1)
Core.CodeInfo(code=Array{Any, (4,)}[
  Expr(:const, :VERSION),
  Expr(:call, :version),

Is that something anyone has seen before? Or maybe loading the MbedTLS stdlib package before libgit2 is making things weird somehow.

@vtjnash
Copy link
Member

vtjnash commented Aug 6, 2019

I've run into something like that before. I think it was libssh2 that had gotten confused about the build state. I didn't know why, so I just wiped away my deps state locally.

@quinnj quinnj force-pushed the jq/mbedtls branch 4 times, most recently from 995a751 to d64c004 Compare August 7, 2019 12:36
@quinnj
Copy link
Member Author

quinnj commented Aug 7, 2019

@KristofferC or @fredrikekre, do either of you (or anyone else!) have any ideas on my failure here? It's trying to call crt_parse_file(joinpath(@__DIR__, "cacert.pem") at runtime, then says the file doesn't exist, even though cacert.pem is indeed located in the src/ directory. I'm guessing there's some issue w/ how we're bringing in the external stdlib package here and symlinking to the usr/share directory? I don't really know, or what to do if we are, but I'd appreciate any tips or ideas if anyone has any. I mean, alternatively, we just do something like read("cacert.pem") in the __init__ function or something and then just parse the CRT from the buffer at runtime.

@quinnj
Copy link
Member Author

quinnj commented Sep 6, 2019

So the CI is looking pretty good on this now, though I'm not sure if the OSX or win32 CI failures should be worrisome or not; they both have "libgit2 library not found" errors. Did I see something change w/ those recently?

What about the overall review process on this; anyone up for taking a look or want to review/provide feedback?

@quinnj
Copy link
Member Author

quinnj commented Sep 12, 2019

img

@quinnj quinnj mentioned this pull request Sep 17, 2019
@staticfloat staticfloat self-assigned this Sep 19, 2019
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

Works for me; I say we merge this and march towards an HTTP.jl stdlib.

@KristofferC
Copy link
Member

Manifests with MbedTLS as a stdlib will probably confuse Pkg on Julia versions before it was made into an stdlib. But I guess we'll see what happens.

@StefanKarpinski
Copy link
Member

We should try it and if it causes confusion, we can teach Pkg to handle it smoothly, so that way it won't be a breaking change. It could maybe print a warning that it can't use the version of MbedTLS requested because MbedTLS has become a stdlib and that it's proceeding with that.

@KristofferC
Copy link
Member

Yeah sure, I was more worried about manifests from 1.4 being used in pre 1.4 julias.

@StefanKarpinski
Copy link
Member

We don't promise anything about compatibility in that direction, so 🤷‍♂

@quinnj
Copy link
Member Author

quinnj commented Nov 6, 2019

Barring major concerns, I'll merge this in the next day or two.

stdlib/Makefile Outdated
PKG_GIT_URL := git://github.com/JuliaLang/Pkg.jl.git
PKG_TAR_URL = https://api.github.com/repos/JuliaLang/Pkg.jl/tarball/$1

MBEDTLS_GIT_URL := git://github.com/JuliaWeb/MbedTLS.jl.git
Copy link
Member

Choose a reason for hiding this comment

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

Should we move this to JuliaLang?

Copy link
Member

Choose a reason for hiding this comment

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

Yes good idea.

base/sysimg.jl Outdated
@@ -28,6 +28,7 @@ let
:Serialization,
:Libdl,
:Markdown,
:MbedTLS,
Copy link
Member Author

Choose a reason for hiding this comment

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

@fredrikekre, if I just remove this, will MbedTLS still be accessible like using MbedTLS, but it won't be precompiled? Does anyone think that's worth it? (i.e. smaller sysimg size since MbedTLS module doesn't really need to be precompiled)

Copy link
Member

Choose a reason for hiding this comment

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

Correct (I think). One advantage is that you can still load a (different) package named MbedTLS :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that seems to work as expected. I'll remove it from this list then.

@quinnj
Copy link
Member Author

quinnj commented Nov 8, 2019

I moved MbedTLS.jl to JuliaLang org and switched the url in this PR.

@quinnj
Copy link
Member Author

quinnj commented Nov 9, 2019

I don't think the CI failures are related.

@ViralBShah
Copy link
Member

I am guessing we don't need this anymore. If so, close?

@quinnj
Copy link
Member Author

quinnj commented Feb 11, 2020

I'd still like to do this; I've just been waiting until 1.4 is released to update it.

@StefanKarpinski
Copy link
Member

How does this relate to the 1.4 release timing? It seems like it's a feature freeze that should matter. It's too late to change this to get into the 1.4 release but it could be changed for 1.5 anytime.

@quinnj
Copy link
Member Author

quinnj commented Feb 14, 2020

Ah, I didn't realize we had already branched 1.4.

@StefanKarpinski
Copy link
Member

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.

7 participants