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.
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
Support user-specified callbacks #443
Support user-specified callbacks #443
Changes from 7 commits
b8f9dcb
9fbdd82
886be8b
5eebd46
60d10d6
035dcc8
346cece
03278a8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 only recently became safe to call
track
more than once on the same file, so this seems OK. But, under what circumstances would these not already be tracked?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.
I don't actually see any reason why they would already be tracked? What if someone does
Revise.add_callback([], Base) do ....
? I'm probably missing something here.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.
I guess I'm reacting to the fact that you're pulling pre-existing files out of the
pkgdata
and adding them tofiles
. Tracking would be on if you'd saidusing MyPkg
and then called this later withadd_callback(..., modules=(MyPkg,),...)
. Stated differently, if they're already inpkgdata
how are they not tracked?Perhaps I don't understand the purpose, and a comment would be helpful.
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.
Ah, now I see what you mean! Yes, this is faulty code then -- I'm trying to guard for the fact that the module may not be under tracking at all yet. Under that logic, we should first call
track(mod)
and only then read the data frompkgdata
, right?If that's the case, I should also add a doctest that tests for this correct behaviour.
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.
Any time you've said
using MyPkg
it should be tracked. Though perhaps thewatch_package
callback (see how it gets registered in__init__
) has to fire first. Is that the issue?I think I've make
track
safe for re-entry (see #433), but I tend to be jittery (probably, needlessly) about things getting done twice and messing up the internal accounting. Maybe poke at this a bit first and try to define more clearly what circumstances require this?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.
The two things that I have in mind are:
Revise
was loaded, e.g.Base
or anything the user added inPackageCompiler
).The first one could potentially be tested by spawning another Julia process. The second one seems almost impossible to test. But in both cases, calling
Revise.track(<module>)
before readingpkgdata
seems like it's the right thing to do -- what do you think?I wasn't thinking about a race condition related to when
watch_package
is being called; do you think we should?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 package precompilation were required for all modules, I could support that, but the existence (even in principle) of non-precompiled packages makes robust support for such a feature impossible. The problematic use case is loading the package, making some changes to the source, and then tracking with Revise: in this case Revise has no way of knowing that what's in the files does not match the code in your running session. Conversely, if you load Revise first, it "defensively" parses the entire package when it's loaded and then it has a baseline to compare any file changes. This is not necessary for precompied packages because I added a source-cache to
*.ji
files in JuliaLang/julia#23898, and did the same for Julia itself in JuliaLang/julia#24120. This means that you can always recover the state of the source at the time at which the*.ji
file was built. But there is no such fix available for non-precompiled packages.So I plan to support this feature by saying "load Revise first, otherwise you're screwed" 😄. More seriously, I could contemplate supporting this specifically for precompiled packages, but we'd need to enforce it.
That's a much more important use-case. Do such files record their source text? If not, we're screwed for now but should fix that ASAP.
Yeah,
track
populates the structure and then you can do stuff with it.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.
@ffevotte this is the thing that needs resolving: in
add_callback
we need to move the call toRevise.track(mod)
to the place before readingpkgdatas[id]
. We don't have to callRevise.track(mod, absname)
after that.After that change, hopefully this is done.