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.

Sign up for GitHub

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

Jump to bottom

Allow hiding stderr during compilecache #37596

Merged
merged 2 commits into from
Sep 21, 2020
Merged

Allow hiding stderr during compilecache #37596

merged 2 commits into from
Sep 21, 2020

Conversation

IanButterworth
Copy link
Member

Required for the error handling in JuliaLang/Pkg.jl#2021

cc. @tkf

@IanButterworth IanButterworth mentioned this pull request Sep 16, 2020
Merged
@tkf
Copy link
Member

tkf commented Sep 16, 2020 •
edited
Loading

Context (just for record): Current implementation of Pkg.precompile as of JuliaLang/Pkg.jl#2018 tries to call Base.compilecache for all packages in Manifest.toml file. This has a problem because some packages may only be required in some specific platform/environment and the package author advises the downstream packages to hide using behind, e.g., if Sys.iswindows() JuliaLang/Pkg.jl#2018 (comment). So, I suggested JuliaLang/Pkg.jl#2018 (comment) to ignore precompilation error from indirect dependencies (which is implemented in JuliaLang/Pkg.jl#2021 by @ianshmean). Doing this in a user-friendly manner requires us to silence messages from the subprocesses. Note that JuliaLang/Pkg.jl#2021 does not ignore precompilation error from direct dependencies (which may be caused by precompilation of indirect dependencies). So, the errors relevant to the user will be correctly surfaced.

(It means that Pkg.precompile has quadratic time-complexity when an error happens. I think it's reasonable to optimize for the happy path for now given that doing this "correctly" is presumably much more challenging JuliaLang/Pkg.jl#2018 (comment). Also, we can still use using TheProblematicPackage to get the precompilation error in linear time.)

@IanButterworth IanButterworth mentioned this pull request Sep 16, 2020
Merged
tkf
tkf reviewed Sep 17, 2020
View reviewed changes
base/loading.jl
@@ -1210,7 +1210,7 @@ end

const MAX_NUM_PRECOMPILE_FILES = 10

function compilecache(pkg::PkgId, path::String)
function compilecache(pkg::PkgId, path::String, show_errors::Bool = true)
Copy link
Member

Choose a reason for hiding this comment

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

I think it'd be better to comment that show_errors is an "internal" interface for Pkg.precompile. It's a bit unclear why we have this from just looking at the code in Base.

@tkf
Copy link
Member

tkf commented Sep 17, 2020

@c42f I think you have opinions/thoughts on nested error message "handling" (in this case just ignoring). I think it's an OK implementation (though obviously not super elegant) but I wonder if you have some alternative ideas.

@c42f
Copy link
Member

c42f commented Sep 18, 2020

I haven't followed the larger story very closely but this seems fine to me, for now. Other options would probably be a lot more complicated.

@tkf
Copy link
Member

tkf commented Sep 18, 2020

Other options would probably be a lot more complicated.

Yeah, that's my thought, too. Thanks for taking a look at it.

tkf
tkf approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@tkf tkf left a comment

Choose a reason for hiding this comment

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

LGTM. I think it's pretty safe to merge but I guess I'll wait for a few days just in case someone (ping @vtjnash and @JeffBezanson) wants to take a look at it.

@tkf tkf requested review from JeffBezanson and vtjnash September 18, 2020 06:44
staticfloat
staticfloat approved these changes Sep 21, 2020
View reviewed changes
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

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

Looks good to me! Rebase and I say merge.

@IanButterworth IanButterworth force-pushed the ib/compilecache_hide_stderr branch from f4b67cd to 05c2ec1 Compare September 21, 2020 18:35
@IanButterworth
Copy link
Member Author

@staticfloat Rebased. Is the aarch64 issue relevant? LoadError: LoadError: "hard kill repl test"

@JeffBezanson JeffBezanson merged commit 03ec50d into JuliaLang:master Sep 21, 2020
@IanButterworth IanButterworth deleted the ib/compilecache_hide_stderr branch September 21, 2020 22:22
@IanButterworth IanButterworth mentioned this pull request Oct 13, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@staticfloat staticfloat staticfloat approved these changes

@tkf tkf tkf approved these changes

@JeffBezanson JeffBezanson Awaiting requested review from JeffBezanson

@vtjnash vtjnash Awaiting requested review from vtjnash

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@IanButterworth @tkf @c42f @staticfloat @JeffBezanson