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 system image incompatibility with LibCURL.cacert #84

Merged
merged 3 commits into from
Jun 24, 2020

Conversation

omus
Copy link
Collaborator

@omus omus commented Jun 24, 2020

When attempting to reference LibCURL.cacert from within a system image the returned path would be "" as this is what MozillaCACerts_jll stores before initialization.

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2020

Codecov Report

Merging #84 into master will increase coverage by 0.70%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #84      +/-   ##
=========================================
+ Coverage    6.81%   7.51%   +0.70%     
=========================================
  Files           2       2              
  Lines         132     133       +1     
=========================================
+ Hits            9      10       +1     
  Misses        123     123              
Impacted Files Coverage Δ
src/LibCURL.jl 41.66% <100.00%> (+5.30%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87ca1d1...104fae2. Read the comment docs.

@omus
Copy link
Collaborator Author

omus commented Jun 24, 2020

I was looking into writing some tests for this but I've been unable to reproduce the failure that existed without this change using something like:

using PackageCompiler
create_sysimage(:LibCURL; sysimage_path="LibCURL.so")
run(`$(Base.julia_cmd()) -JLibCURL.so -E '@assert !isempty(LibCURL.cacert)'`)

I may look into this further but I don't think it's essential to add a test here if I can confirm this fixes the issue.

@iamed2
Copy link

iamed2 commented Jun 24, 2020

Globals should be set as const Ref values rather than global

@omus
Copy link
Collaborator Author

omus commented Jun 24, 2020

Globals should be set as const Ref values rather than global

I don't disagree. However, I want this change to be non-breaking and using a Ref requires this to be a breaking change then. Also since this is just a copy of what MozillaCACerts_jll is doing probably an issue should be created there.

@omus
Copy link
Collaborator Author

omus commented Jun 24, 2020

I've made an issue against BinaryBuilder.jl: JuliaPackaging/BinaryBuilder.jl#829

@omus
Copy link
Collaborator Author

omus commented Jun 24, 2020

I have confirmed that this fixes the incompatibility with the system image.

@omus omus merged commit d018fe7 into master Jun 24, 2020
@omus omus deleted the cv/sysimage-compat branch June 24, 2020 18:07
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.

5 participants