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

[backports-release-1.11] fix pkgdir during inits on 1.11 #56546

Open
wants to merge 6 commits into
base: backports-release-1.11
Choose a base branch
from

Conversation

IanButterworth
Copy link
Member

Proposed fix for #56077 on 1.11

julia> using MWE
[ Info: Precompiling MWE [f3b207a7-3b3b-4b3b-8b3b-3b3b3b3b3b3b] (cache misses: include_dependency fsize change (1))
MWE
/Users/ian/Documents/GitHub/mwe

@IanButterworth IanButterworth added the packages Package management and loading label Nov 13, 2024
Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I believe you cannot move this here because we rely on the require lock remaining held during this function for safety

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

Oh, I see, this is actually moving it later? I guess that seems okay

@KristofferC
Copy link
Member

Can someone explain why you want to call this in init?

@IanButterworth
Copy link
Member Author

I don't have a defense. If you want to look into the issue in the ecosystem JuliaIO/JLD2.jl#610

But I don't think we should be fixing this by changing user code. It works on 1.10 and master

@IanButterworth
Copy link
Member Author

@vtjnash this appears to now avoid a deadlock that tests expected

precompile (1) |         failed at 2024-11-13T14:47:42.184
Test Failed at /var/folders/1z/jf841bdj73bdj3vk7kc7f_3w0000gn/T/jl_INOnAw/Baz26028.jl:3
  Expression: #= /var/folders/1z/jf841bdj73bdj3vk7kc7f_3w0000gn/T/jl_INOnAw/Baz26028.jl:4 =# @eval import Foo26028.Bar26028.x
    Expected: ConcurrencyViolationError("deadlock detected in loading Foo26028 -> Foo26028")
  No exception thrown

Related issues
#26028
#28416

I think that's a good thing?

@vtjnash
Copy link
Member

vtjnash commented Nov 13, 2024

That means you've introduced a new data race. Although I don't see precisely why, I think it is because of the expected failures without the fixes for it from #56329

@IanButterworth
Copy link
Member Author

I can't figure out why either.

I think it is because of the expected failures without the fixes for it from #56329

I don't follow this, sorry.

@vtjnash
Copy link
Member

vtjnash commented Nov 14, 2024

The test ensures that the fixes in #56329 are present

@IanButterworth
Copy link
Member Author

So we mark the test as broken on 1.11 ?

@IanButterworth
Copy link
Member Author

@vtjnash I've done that, but this does need approval.

@IanButterworth
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages Package management and loading
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants