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

clear out cache variables when loading the package #34

Closed
wants to merge 1 commit into from

Conversation

KristofferC
Copy link
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
Copy link

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.

@@ -74,7 +74,7 @@ const BSD_CA_ROOTS = [
]

const SYSTEM_CA_ROOTS_LOCK = ReentrantLock()
const SYSTEM_CA_ROOTS = Ref{String}()
const SYSTEM_CA_ROOTS = Ref{Union{String,Nothing}}(nothing)
Copy link
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
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
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
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