Skip to content

clear out cache variables when loading the package#34

Closed
KristofferC wants to merge 1 commit intomasterfrom
kc/global_caches
Closed

clear out cache variables when loading the package#34
KristofferC wants to merge 1 commit intomasterfrom
kc/global_caches

Conversation

@KristofferC
Copy link
Copy Markdown
Member

This is required on 1.10 where Downloads and NetworkOptions are in the sysimage (see JuliaLang/julia#53339) but it seems like a good idea here anyway in case someone adds a precompile workload to this package.

I thought about using atexit hooks for this to avoid the __init__ but that would require JuliaLang/julia#51849 (AFAIU) and we don't have that on 1.10.

@KristofferC KristofferC changed the title clear out cache variables before serializing clear out cache variables when loading the package Feb 15, 2024
This is required on 1.10 where Downloads and NetworkOptions are in the sysimage (see JuliaLang/julia#53339) but it seems like a good idea here anyway in case someone adds a precompile workload to this package
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (aab83e5) 95.52% compared to head (2eb7348) 97.10%.

Files Patch % Lines
src/ca_roots.jl 88.23% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #34      +/-   ##
==========================================
+ Coverage   95.52%   97.10%   +1.57%     
==========================================
  Files           4        4              
  Lines         134      138       +4     
==========================================
+ Hits          128      134       +6     
+ Misses          6        4       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


const SYSTEM_CA_ROOTS_LOCK = ReentrantLock()
const SYSTEM_CA_ROOTS = Ref{String}()
const SYSTEM_CA_ROOTS = Ref{Union{String,Nothing}}(nothing)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
const SYSTEM_CA_ROOTS = Ref{Union{String,Nothing}}(nothing)
const SYSTEM_CA_ROOTS = Ref{Union{String, Nothing}}(nothing)

The pickiest of nits. Just because the other one has the space.

for line in eachline(path)
if line == BEGIN_CERT_REGULAR
roots = path
openssl_only = false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why does this change more of the logic than just SYSTEM_CA_ROOTS? Was there another issue to fix here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll change it back, I just wanted it to be more similar to how it was changed in BUNDLED_KNOWN_HOSTS_FILE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's fine, I just wasn't sure about the openssl_only part, which looks like a logic change to me.

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