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

Hide loading internals in LoadError / precompilation errors #55816

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion base/errorshow.jl
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,14 @@ end

function showerror(io::IO, ex::LoadError, bt; backtrace=true)
!isa(ex.error, LoadError) && print(io, "LoadError: ")
if bt isa StackTraces.StackTrace
# Remove the internals of how precompilation is set up
StackTraces.remove_frames!(bt, :include_package_for_output; reverse=true, n=1)
Copy link
Member

@topolarity topolarity Sep 20, 2024

Choose a reason for hiding this comment

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

This feels a bit error-prone - in particular, doesn't it require ruling out recursion in the loading code?

I wonder if rethrow(truncate_bt=true) / throw(e; truncate_bt=true) or Base.truncate_catch_backtrace(...) or a similar functionality would be better behaved

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I guess the most certain part of this PR is what the result should look like. How that's achieved I was less clear on a good design.

What would be a good test case for recursion?

Copy link
Member Author

Choose a reason for hiding this comment

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

@topolarity if you had a particular loading pattern in mind it'd be great to understand it better.

Copy link
Member Author

Choose a reason for hiding this comment

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

By recursion do you mean an included file that has an include in it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Bump

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for the delay here @IanButterworth, I've been sitting on some changes to our stack trace handling that I wanted to upstream so that we can do this properly (without scanning for function names, etc.)

Let me get those open and come back to this early next week 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect. Thanks

end
showerror(io, ex.error, bt, backtrace=backtrace)
print(io, "\nin expression starting at $(ex.file):$(ex.line)")
if ex.file !== "stdin"
print(io, "\nin expression starting at $(ex.file):$(ex.line)")
end
end
showerror(io::IO, ex::LoadError) = showerror(io, ex, [])

Expand Down
13 changes: 12 additions & 1 deletion base/loading.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3044,6 +3044,17 @@ end

const MAX_NUM_PRECOMPILE_FILES = Ref(10)

struct PackagePrecompileError <: Exception
pkg::PkgId
path::String
end
function showerror(io::IO, ex::PackagePrecompileError, bt; backtrace)
showerror(io, ex)
end
function showerror(io::IO, ex::PackagePrecompileError)
print(io, "Failed to precompile $(repr("text/plain", ex.pkg)) to $(repr(ex.path)).")
end

function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, internal_stdout::IO = stdout,
keep_loaded_modules::Bool = true; flags::Cmd=``, cacheflags::CacheFlags=CacheFlags(),
reasons::Union{Dict{String,Int},Nothing}=Dict{String,Int}(), isext::Bool=false)
Expand Down Expand Up @@ -3166,7 +3177,7 @@ function compilecache(pkg::PkgId, path::String, internal_stderr::IO = stderr, in
if p.exitcode == 125
return PrecompilableError()
else
error("Failed to precompile $(repr("text/plain", pkg)) to $(repr(tmppath)).")
throw(PackagePrecompileError(pkg, tmppath))
end
end

Expand Down
16 changes: 8 additions & 8 deletions base/precompilation.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module Precompilation

using Base: PkgId, UUID, SHA1, parsed_toml, project_file_name_uuid, project_names,
project_file_manifest_path, get_deps, preferences_names, isaccessibledir, isfile_casesensitive,
base_project
base_project, PackagePrecompileError

# This is currently only used for pkgprecompile but the plan is to use this in code loading in the future
# see the `kc/codeloading2.0` branch
Expand Down Expand Up @@ -330,14 +330,14 @@ end


############
struct PkgPrecompileError <: Exception
struct PrecompilePkgsError <: Exception
msg::String
end
Base.showerror(io::IO, err::PkgPrecompileError) = print(io, err.msg)
Base.showerror(io::IO, err::PkgPrecompileError, bt; kw...) = Base.showerror(io, err) # hide stacktrace
Base.showerror(io::IO, err::PrecompilePkgsError) = print(io, err.msg)
Base.showerror(io::IO, err::PrecompilePkgsError, bt; kw...) = Base.showerror(io, err) # hide stacktrace

# This needs a show method to make `julia> err` show nicely
Base.show(io::IO, err::PkgPrecompileError) = print(io, "PkgPrecompileError: ", err.msg)
Base.show(io::IO, err::PrecompilePkgsError) = print(io, "PrecompilePkgsError: ", err.msg)

import Base: StaleCacheKey

Expand Down Expand Up @@ -824,7 +824,7 @@ function precompilepkgs(pkgs::Vector{String}=String[];
# @show err
close(std_pipe.in) # close pipe to end the std output monitor
wait(t_monitor)
if err isa ErrorException || (err isa ArgumentError && startswith(err.msg, "Invalid header in cache file"))
if err isa PackagePrecompileError || (err isa ArgumentError && startswith(err.msg, "Invalid header in cache file"))
failed_deps[pkg_config] = (strict || is_direct_dep) ? string(sprint(showerror, err), "\n", strip(get(std_outputs, pkg_config, ""))) : ""
delete!(std_outputs, pkg_config) # so it's not shown as warnings, given error report
!fancyprint && lock(print_lock) do
Expand Down Expand Up @@ -946,15 +946,15 @@ function precompilepkgs(pkgs::Vector{String}=String[];
plural1 = length(failed_deps) == 1 ? "y" : "ies"
println(io, " ", color_string("$(length(failed_deps))", Base.error_color()), " dependenc$(plural1) errored.")
println(io, " For a report of the errors see `julia> err`. To retry use `pkg> precompile`")
setglobal!(Base.MainInclude, :err, PkgPrecompileError(err_msg))
setglobal!(Base.MainInclude, :err, PrecompilePkgsError(err_msg))
else
# auto-precompilation shouldn't throw but if the user can't easily access the
# error messages, just show them
print(io, "\n", err_msg)
end
else
println(io)
throw(PkgPrecompileError(err_msg))
throw(PrecompilePkgsError(err_msg))
end
end
end
Expand Down
19 changes: 15 additions & 4 deletions base/stacktraces.jl
Original file line number Diff line number Diff line change
Expand Up @@ -198,15 +198,26 @@ Base.@constprop :none function stacktrace(c_funcs::Bool=false)
end

"""
remove_frames!(stack::StackTrace, name::Symbol)
remove_frames!(stack::StackTrace, name::Symbol; reverse::Bool=false)

Takes a `StackTrace` (a vector of `StackFrames`) and a function name (a `Symbol`) and
removes the `StackFrame` specified by the function name from the `StackTrace` (also removing
all frames above the specified function). Primarily used to remove `StackTraces` functions
from the `StackTrace` prior to returning it.
from the `StackTrace` prior to returning it (or after if `reverse` is `true`).
"""
function remove_frames!(stack::StackTrace, name::Symbol)
deleteat!(stack, 1:something(findlast(frame -> frame.func == name, stack), 0))
function remove_frames!(stack::StackTrace, name::Symbol; reverse::Bool=false, n::Int=0)
if reverse
f = findfirst(frame -> frame.func == name, stack)
if f !== nothing
stack_length = length(stack)
deleteat!(stack, (f - n):stack_length)
end
else
f = findlast(frame -> frame.func == name, stack)
if f !== nothing
deleteat!(stack, 1:(f + n))
end
end
return stack
end

Expand Down
2 changes: 1 addition & 1 deletion test/gcext/gcext-test.jl
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ end
Base.compilecache(Base.identify_package("DependsOnForeign"))

push!(LOAD_PATH, joinpath(@__DIR__, "ForeignObjSerialization"))
@test_throws ErrorException Base.compilecache(Base.identify_package("ForeignObjSerialization"), Base.DevNull())
@test_throws Base.PackagePrecompileError Base.compilecache(Base.identify_package("ForeignObjSerialization"), Base.DevNull())
pop!(LOAD_PATH)

(@eval (using Foreign))
Expand Down