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

make package cache interit permission from source #34573

Merged
merged 2 commits into from
Feb 3, 2020
Merged

make package cache interit permission from source #34573

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2f4689
make package cache interit permission from source
bicycle1885 Jan 29, 2020
1f5d2b3
remove temporary directries
bicycle1885 Jan 30, 2020
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
2 changes: 2 additions & 0 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1264,6 +1264,8 @@ function compilecache(pkg::PkgId, path::String)
open(cachefile, "a+") do f
write(f, _crc32c(seekstart(f)))
end
# inherit permission from the source file
chmod(cachefile, filemode(path) & 0o777)
elseif p.exitcode == 125
return PrecompilableError()
else
Expand Down
21 changes: 21 additions & 0 deletions test/precompile.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -797,4 +797,25 @@ let
end
end

# Issue #25971
let
load_path = mktempdir()
load_cache_path = mktempdir()
try
pushfirst!(LOAD_PATH, load_path)
pushfirst!(DEPOT_PATH, load_cache_path)
sourcefile = joinpath(load_path, "Foo25971.jl")
write(sourcefile, "module Foo25971 end")
chmod(sourcefile, 0o666)
cachefile = Base.compilecache(Base.PkgId("Foo25971"))
@test filemode(sourcefile) == filemode(cachefile)
chmod(sourcefile, 0o600)
cachefile = Base.compilecache(Base.PkgId("Foo25971"))
@test filemode(sourcefile) == filemode(cachefile)
finally
rm(load_path, recursive=true)
rm(load_cache_path, recursive=true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Even though CI is passing, should the updates to LOAD_PATH and DEPOT_PATH be reversed again?

Suggested change
rm(load_cache_path, recursive=true)
rm(load_cache_path, recursive=true)
filter!(s -> s != load_path, LOAD_PATH)
filter!(s -> s != load_cache_path, DEPOT_PATH)

Copy link
Member

Choose a reason for hiding this comment

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

I should suggest using popfirst!(LOAD_PATH) instead.

Copy link
Contributor

@jonas-schulze jonas-schulze Jan 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've thought about that, too, but using filter! would seems to be more robust in case of a failure in between or even before the pushfirst!s, or should the tests be run concurrently some time in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Another way is to copy before running the code and then copy! it back when done.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed those paths as @jonas-schulze suggested. Thank you.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW, mktempdir now automatically deletes the folder when the process exits.

Copy link
Member

Choose a reason for hiding this comment

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

Still a best practice to delete when done using.

end
end

end # !withenv