Skip to content

Revise overloaded temp methods #150

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

Open
rofinn wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Revise overloaded temp methods #150

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d73f9e4
Revise overloaded temp methods to support new 1.3+ kwargs (e.g., pref…
rofinn Nov 17, 2021
eb567e4
Add no-arg tempname fallback for pre 1.4 Julia releases.
rofinn Nov 17, 2021
8b90a94
Backwards compatible method works fine on 1.4+ as well.
rofinn Nov 17, 2021
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
61 changes: 32 additions & 29 deletions src/path.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -687,8 +687,8 @@ Download a file from the remote `url` and save it to the `localfile` path.
NOTE: Not downloading into a `localfile` directory matches the base Julia behaviour.
https://github.com/rofinn/FilePathsBase.jl/issues/48
"""
function Base.download(url::AbstractString, localfile::P) where P <: AbstractPath
mktemp(P) do fp, io
function Base.download(url::AbstractString, localfile::AbstractPath)
mktmp() do fp, io
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

mktemp(P) previously returned a SystemPath by default. We've changed this, so we've made this explicit to avoid infinite recursion on the download call below.

download(url, fp)
cp(fp, localfile; force=false)
end
Expand Down Expand Up @@ -767,41 +767,37 @@ Base.write(fp::AbstractPath, x) = open(io -> write(io, x), fp, "w")
# Default `touch` will just write an empty string to a file
Base.touch(fp::AbstractPath) = write(fp, "")

Base.tempname(::Type{<:AbstractPath}) = Path(tempname())
tmpname() = tempname(SystemPath)

Base.tempdir(::Type{<:AbstractPath}) = Path(tempdir())
tmpdir() = tempdir(SystemPath)

Base.mktemp(P::Type{<:AbstractPath}) = mktemp(tempdir(P))
mktmp() = mktemp(SystemPath)

Base.mktemp(fn::Function, P::Type{<:AbstractPath}) = mktemp(fn, tempdir(P))
mktmp(fn::Function) = mktemp(fn, SystemPath)

Base.mktempdir(P::Type{<:AbstractPath}) = mktempdir(tempdir(P))
mktmpdir() = mktempdir(SystemPath)

Base.mktempdir(fn::Function, P::Type{<:AbstractPath}) = mktempdir(fn, tempdir(P))
mktmpdir(fn::Function) = mktempdir(fn, SystemPath)
Base.tempname(P::Type{<:AbstractPath}; kwargs...) = tempname(tempdir(P); kwargs...)
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

The default behaviour for tempname now calls tempdir(P), so we require that all new path types define that method for their specific type.

Base.mktemp(P::Type{<:AbstractPath}; kwargs...) = mktemp(tempdir(P); kwargs...)
Base.mktemp(fn::Function, P::Type{<:AbstractPath}; kwargs...) = mktemp(fn, tempdir(P); kwargs...)
Base.mktempdir(P::Type{<:AbstractPath}; kwargs...) = mktempdir(tempdir(P); kwargs...)
Base.mktempdir(fn::Function, P::Type{<:AbstractPath}; kwargs...) = mktempdir(fn, tempdir(P); kwargs...)

function Base.tempname(parent::AbstractPath; prefix="jl_", cleanup=true)
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

prefix was also added in 1.2 (though not as consistently).

isdir(parent) || throw(ArgumentError("$(repr(parent)) is not a directory"))
fp = parent / string(prefix, uuid1())
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

Switching to uuid1 over uuid4. For older releases of Julia, these function depended on the GLOBAL_RNG and could in theory result in collisions if folks were resetting while generating temp files and directories.

@assert !exists(fp)
cleanup && temp_cleanup(fp)
return fp
end

function Base.mktemp(parent::AbstractPath)
fp = parent / string(uuid4())
function Base.mktemp(parent::AbstractPath; kwargs...)
fp = tempname(parent; kwargs...)
# touch the file in case `open` isn't implement for the path and
# we're buffering locally.
touch(fp)
io = open(fp, "w+")
return fp, io
end

function Base.mktempdir(parent::AbstractPath)
fp = parent / string(uuid4())
function Base.mktempdir(parent::AbstractPath; kwargs...)
fp = tempname(parent; kwargs...)
mkdir(fp)
return fp
end

function Base.mktemp(fn::Function, parent::AbstractPath)
(tmp_fp, tmp_io) = mktmp(parent)
function Base.mktemp(fn::Function, parent::AbstractPath; kwargs...)
(tmp_fp, tmp_io) = mktemp(parent; kwargs...)
try
fn(tmp_fp, tmp_io)
finally
Expand All @@ -810,17 +806,24 @@ function Base.mktemp(fn::Function, parent::AbstractPath)
end
end

function Base.mktempdir(fn::Function, parent::AbstractPath)
tmpdir = mktmpdir(parent)
function Base.mktempdir(fn::Function, parent::AbstractPath; kwargs...)
tmpdir = mktmpdir(parent; kwargs...)
try
fn(tmpdir)
finally
rm(tmpdir, recursive=true)
end
end

mktmp(arg1, args...) = mktemp(arg1, args...)
mktmpdir(arg1, args...) = mktempdir(arg1, args...)
mktmp(arg1, args...; kwargs...) = mktemp(arg1, args...; kwargs...)
mktmpdir(arg1, args...; kwargs...) = mktempdir(arg1, args...; kwargs...)

function temp_cleanup(fp::AbstractPath)
atexit() do
# Might not work in all cases, but default to recursively deleting the path on exit
rm(fp; force=true, recursive=true)
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This was the specific suggestion made in #141. It might not always work, but it seems better than nothing?

end
end

"""
isdescendant(fp::P, asc::P) where {P <: AbstractPath} -> Bool
Expand Down
57 changes: 38 additions & 19 deletions src/system.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,8 @@ function isexecutable(fp::SystemPath)
return (
isexecutable(s.mode, :ALL) ||
isexecutable(s.mode, :OTHER) ||
( usr.uid == s.user.uid && isexecutable(s.mode, :USER) ) ||
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

My editor fixed some formatting issues from now till my next commend.

( usr.gid == s.group.gid && isexecutable(s.mode, :GROUP) )
(usr.uid == s.user.uid && isexecutable(s.mode, :USER)) ||
(usr.gid == s.group.gid && isexecutable(s.mode, :GROUP))
)
end

Expand All @@ -130,8 +130,8 @@ function Base.iswritable(fp::SystemPath)
return (
iswritable(s.mode, :ALL) ||
iswritable(s.mode, :OTHER) ||
( usr.uid == s.user.uid && iswritable(s.mode, :USER) ) ||
( usr.gid == s.group.gid && iswritable(s.mode, :GROUP) )
(usr.uid == s.user.uid && iswritable(s.mode, :USER)) ||
(usr.gid == s.group.gid && iswritable(s.mode, :GROUP))
)
end

Expand All @@ -147,8 +147,8 @@ function Base.isreadable(fp::SystemPath)
return (
isreadable(s.mode, :ALL) ||
isreadable(s.mode, :OTHER) ||
( usr.uid == s.user.uid && isreadable(s.mode, :USER) ) ||
( usr.gid == s.group.gid && isreadable(s.mode, :GROUP) )
(usr.uid == s.user.uid && isreadable(s.mode, :USER)) ||
(usr.gid == s.group.gid && isreadable(s.mode, :GROUP))
)
end

Expand Down Expand Up @@ -184,7 +184,7 @@ function Base.cd(fn::Function, dir::SystemPath)
try
cd(dir)
fn()
finally
finally
cd(old)
end
end
Expand All @@ -198,17 +198,17 @@ function Base.mkdir(fp::T; mode=0o777, recursive=false, exist_ok=false) where T<
end
else
if hasparent(fp) && !exists(parent(fp))
if recursive
mkdir(parent(fp); mode=mode, recursive=recursive, exist_ok=exist_ok)
else
error(
"The parent of $fp does not exist. " *
"Pass recursive=true to create it."
)
end
if recursive
mkdir(parent(fp); mode=mode, recursive=recursive, exist_ok=exist_ok)
else
error(
"The parent of $fp does not exist. " *
"Pass recursive=true to create it."
)
end
end

return parse(T, mkdir(string(fp), mode=mode))
return parse(T, mkdir(string(fp), mode=mode))
end
end

Expand All @@ -231,13 +231,32 @@ end
function Base.rm(fp::SystemPath; kwargs...)
rm(string(fp); kwargs...)
end

Base.touch(fp::T) where {T<:SystemPath} = parse(T, touch(string(fp)))
function Base.mktemp(parent::T) where T<:SystemPath
fp, io = mktemp(string(parent))

Base.tempdir(::Type{<:SystemPath}) = Path(tempdir())
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

Moved the SystemPath specific defaults to the system.jl file.

Base.tempname(::Type{<:SystemPath}) = Path(tempname())

function Base.tempname(parent::T; kwargs...) where {T<:SystemPath}
return parse(T, tempname(string(parent); kwargs...))
end

function Base.mktemp(parent::T; kwargs...) where {T<:SystemPath}
fp, io = mktemp(string(parent); kwargs...)
return parse(T, fp), io
end

Base.mktempdir(parent::T) where {T<:SystemPath}= parse(T, mktempdir(string(parent)))
function Base.mktempdir(parent::T; kwargs...) where {T<:SystemPath}
return parse(T, mktempdir(string(parent); kwargs...))
end

# Some extra default/utilityy functions
tmpdir() = tempdir(SystemPath)
tmpname(; kwargs...) = tempname(SystemPath; kwargs...)
mktmp() = mktemp(SystemPath)
mktmp(fn::Function) = mktemp(fn, SystemPath)
mktmpdir() = mktempdir(SystemPath)
mktmpdir(fn::Function) = mktempdir(fn, SystemPath)

"""
chown(fp::SystemPath, user::AbstractString, group::AbstractString; recursive=false)
Expand Down
1 change: 1 addition & 0 deletions test/testpkg.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,6 @@ Base.read(fp::TestPath) = read(test2posix(fp))
Base.write(fp::TestPath, x) = write(test2posix(fp), x)
Base.chown(fp::TestPath, args...; kwargs...) = chown(test2posix(fp), args...; kwargs...)
Base.chmod(fp::TestPath, args...; kwargs...) = chmod(test2posix(fp), args...; kwargs...)
Base.tempdir(::Type{TestPath}) = posix2test(tempdir(PosixPath))
Copy link
Owner Author

@rofinn rofinn Nov 17, 2021

Choose a reason for hiding this comment

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

Example of needing to add a Base.tempdir overload.


end