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

Follow up changes for InitError #12742

Merged
merged 2 commits into from
Aug 27, 2015
Merged

Conversation

jdlangs
Copy link
Contributor

@jdlangs jdlangs commented Aug 22, 2015

Per the comments in #12576, this no longer escapes InitErrors from being wrapped in LoadErrors.

I also wrapped all the Base submodule __init__ functions that use external libraries in try ... catch so they don't throw but I'm not sure if it's the best approach to take.

ccall((:MKL_Set_Interface_Layer, Base.libblas_name), Void, (Cint,), USE_BLAS64 ? 1 : 0)
end
catch ex
#Base.showerror_nostdio(ex,
Copy link
Contributor

Choose a reason for hiding this comment

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

was this not able to show the details from the exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just mistakenly didn't revert some of my last tests. Fixing now.

@JeffBezanson
Copy link
Member

Hmm, needing to wrap all of these __init__ functions in try-catch is tedious. I know --- let's just wrap all of them from one place in module.c! Oh wait...

end
try
if gmp_version().major != GMP_VERSION.major || gmp_bits_per_limb() != GMP_BITS_PER_LIMB
error(string("The dynamically loaded GMP library (version $(gmp_version()) with __gmp_bits_per_limb == $(gmp_bits_per_limb()))\n",
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this just be warn(string(.... The try... catch seems strange, when the only possible exception comes from an explicit error()

Please verify if warn work in __init__, there might be issues with STDOUT there too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not just that error call, but any ccall into libgmp that might throw. The goal is to let Base load even when any of the shared libraries are actually missing, though I don't know how that situation might arise.

warn was the first thing I tried and it didn't work which led to the new showerror function.

Copy link
Contributor

Choose a reason for hiding this comment

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

though I don't know how that situation might arise.

people embedding julia into a different language or wanting to deploy standalone executables might not always need bigint's

@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 22, 2015

It should provide good incentive to not do shared library loading in Base if possible.

Another option would be to catch all __init__ exceptions from precompiled modules here and rethrow the ones that didn't have Base as their parent. Are there any cases where the Base modules are not precompiled?

@tkelman
Copy link
Contributor

tkelman commented Aug 26, 2015

Bump. Is this ready?

@jdlangs
Copy link
Contributor Author

jdlangs commented Aug 26, 2015

It would be nice to leave a note somewhere explaining why the try/catchs are there. Any suggestions other than a comment on each module?

end
catch ex
Base.showerror_nostdio(ex,
"WARNING: Error during initialization of module LinAlg")
Copy link
Member

Choose a reason for hiding this comment

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

Missing the :\n at the end of this message. However I think it's better to print the formatting stuff (colon and newline) where it's needed rather than making it part of the message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving it to the showerror function now.

Prevents them from throwing any InitError exceptions that will not get
handled
@tkelman tkelman mentioned this pull request Aug 27, 2015
13 tasks
JeffBezanson added a commit that referenced this pull request Aug 27, 2015
Follow up changes for InitError
@JeffBezanson JeffBezanson merged commit 1e42a47 into JuliaLang:master Aug 27, 2015
@coveralls
Copy link

Coverage Status

Coverage decreased (-54.7%) to 27.181% when pulling fe0337e on jdlangs:init_exceptions into 8b95c0d on JuliaLang:master.

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