-
-
Notifications
You must be signed in to change notification settings - Fork 269
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
Allow failures of indirect dependencies during parallel precompillation #2021
Merged
staticfloat
merged 25 commits into
JuliaLang:master
from
IanButterworth:ib/parallel_precomp_optional
Sep 23, 2020
Merged
Changes from all commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
1498ae0
provide parallel kwarg in precompile to select previous behavior
IanButterworth 0932b35
only throw errors during precomp of top-level deps
IanButterworth 7d54ce8
hide stderr
IanButterworth 80077bc
remove option to revert to old precompile method
IanButterworth 7e9c1b2
use the term "indirect dependency"
IanButterworth e227bc2
don't ignore stdlib if not loaded
IanButterworth b842842
rework around single depsmap
IanButterworth 19a7d01
suggestion
IanButterworth 8289dc3
Apply suggestions from code review
IanButterworth e1af932
switch to Base.is_sysimage
IanButterworth f84924a
tweak error reporting
IanButterworth 7765612
Trigger CI
IanButterworth 79d976a
Trigger CI
IanButterworth 309044b
Trigger CI
IanButterworth 039f64e
Trigger CI
IanButterworth 8fecd9a
Apply suggestions from code review
IanButterworth 87e540b
Update src/API.jl
IanButterworth 86f6364
Update src/API.jl
tkf 88b5a71
use single tomlcache during sequential locate_package calls
IanButterworth 18c5351
avoid deadlock when __precompile__(false)
IanButterworth 2a5c814
use tomlcache during async is_stale check
IanButterworth 3affc9f
make _is_stale a standalone function
IanButterworth 8cada29
remove unnecessary wait
IanButterworth 17a0934
remove unnecessary type declaration
IanButterworth 6270541
Merge branch 'master' into ib/parallel_precomp_optional
IanButterworth File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to rely on
Base.TOMLCache
to be "async
-safe" (which is, BTW, pretty easy to break; e.g., a@debug
can ruin this property), I don't think it's necessary to move_is_stale
out. It's actually where closure is useful. Although there is nothing bad in factoring out_is_stale
this way.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@staticfloat did some probing of the list of toml files that were accessed when no tomlcaches were provided, and it was just the current environment manifest and project files (2 files), so the thinking was that it was async safe as it's read-only after the first use in the for-loop. Would that be broken by
@debug
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to resolve this, and merge, to fix #2035
@fredrikekre @KristofferC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parsed_toml(cache::TOMLCache, project_file::String)
reusesTOML.Parser
: https://github.com/JuliaLang/julia/blob/2a36f83ce9bdfd17c572472f60b830aedcf181f0/base/loading.jl#L174-L175So, for example,
TOML.parse(::TOML.Parser)
has to be async-safe. It probably is and likely be so forever but relying on "likely" is not the best approach especially in concurrent programming. For example, maybe somebody will implementTOML.parse(::IO)
at some point and the parsing can happen in a streaming fashion.(It's also a bit concerning that the I/O
read(project_file, String)
is happening inget!
but that's probably handled by the.age
field inDict
.)Anyway, I don't think my comment should block this PR. We can fix it in
Base
if we want.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, merging this as-is seems like the thing to do for now