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

Case sensitive Pkg operations on case insensitive filesystems #18210

Closed
wants to merge 2 commits into from
Closed

Case sensitive Pkg operations on case insensitive filesystems #18210

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f8dbc7c
make Pkg operations case sensitive on case-insensitive filesystems (f…
stevengj Aug 22, 2016
3262c17
Add additional Pkg case sen path comparisons and add tests
jakebolewski Aug 23, 2016
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
78 changes: 0 additions & 78 deletions base/loading.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,84 +2,6 @@

# Base.require is the implementation for the `import` statement

# Cross-platform case-sensitive path canonicalization

if is_unix() && !is_apple()
# assume case-sensitive filesystems, don't have to do anything
isfile_casesensitive(path) = isfile(path)
elseif is_windows()
# GetLongPathName Win32 function returns the case-preserved filename on NTFS.
function isfile_casesensitive(path)
isfile(path) || return false # Fail fast
Filesystem.longpath(path) == path
end
elseif is_apple()
# HFS+ filesystem is case-preserving. The getattrlist API returns
# a case-preserved filename. In the rare event that HFS+ is operating
# in case-sensitive mode, this will still work but will be redundant.

# Constants from <sys/attr.h>
const ATRATTR_BIT_MAP_COUNT = 5
const ATTR_CMN_NAME = 1
const BITMAPCOUNT = 1
const COMMONATTR = 5
const FSOPT_NOFOLLOW = 1 # Don't follow symbolic links

const attr_list = zeros(UInt8, 24)
attr_list[BITMAPCOUNT] = ATRATTR_BIT_MAP_COUNT
attr_list[COMMONATTR] = ATTR_CMN_NAME

# This essentially corresponds to the following C code:
# attrlist attr_list;
# memset(&attr_list, 0, sizeof(attr_list));
# attr_list.bitmapcount = ATTR_BIT_MAP_COUNT;
# attr_list.commonattr = ATTR_CMN_NAME;
# struct Buffer {
# u_int32_t total_length;
# u_int32_t filename_offset;
# u_int32_t filename_length;
# char filename[max_filename_length];
# };
# Buffer buf;
# getattrpath(path, &attr_list, &buf, sizeof(buf), FSOPT_NOFOLLOW);
function isfile_casesensitive(path)
isfile(path) || return false
path_basename = String(basename(path))
local casepreserved_basename
const header_size = 12
buf = Array{UInt8}(length(path_basename) + header_size + 1)
while true
ret = ccall(:getattrlist, Cint,
(Cstring, Ptr{Void}, Ptr{Void}, Csize_t, Culong),
path, attr_list, buf, sizeof(buf), FSOPT_NOFOLLOW)
systemerror(:getattrlist, ret ≠ 0)
filename_length = unsafe_load(
convert(Ptr{UInt32}, pointer(buf) + 8))
if (filename_length + header_size) > length(buf)
resize!(buf, filename_length + header_size)
continue
end
casepreserved_basename =
view(buf, (header_size+1):(header_size+filename_length-1))
break
end
# Hack to compensate for inability to create a string from a subarray with no allocations.
path_basename.data == casepreserved_basename && return true

# If there is no match, it's possible that the file does exist but HFS+
# performed unicode normalization. See https://developer.apple.com/library/mac/qa/qa1235/_index.html.
isascii(path_basename) && return false
normalize_string(path_basename, :NFD).data == casepreserved_basename
end
else
# Generic fallback that performs a slow directory listing.
function isfile_casesensitive(path)
isfile(path) || return false
dir, filename = splitdir(path)
any(readdir(dir) .== filename)
end
end

function try_path(prefix::String, base::String, name::String)
path = joinpath(prefix, name)
isfile_casesensitive(path) && return abspath(path)
Expand Down
62 changes: 32 additions & 30 deletions base/pkg/entry.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import ..Reqs, ..Read, ..Query, ..Resolve, ..Cache, ..Write, ..Dir
import ...LibGit2
importall ...LibGit2
import ...Pkg.PkgError
using ..Types
using ..Types, Base.Filesystem

macro recover(ex)
quote
Expand Down Expand Up @@ -48,25 +48,23 @@ end

function add(pkg::AbstractString, vers::VersionSet)
outdated = :maybe
@sync begin
@async if !edit(Reqs.add,pkg,vers)
ispath(pkg) || throw(PkgError("unknown package $pkg"))
info("Nothing to be done")
end
branch = Dir.getmetabranch()
outdated = with(GitRepo, "METADATA") do repo
if LibGit2.branch(repo) == branch
if LibGit2.isdiff(repo, "origin/$branch")
outdated = :yes
else
try
LibGit2.fetch(repo)
outdated = LibGit2.isdiff(repo, "origin/$branch") ? (:yes) : (:no)
end
end
if !edit(Reqs.add,pkg,vers)
ispath_casesensitive(pkg) || throw(PkgError("unknown package $pkg"))
info("Nothing to be done")
Copy link
Member

Choose a reason for hiding this comment

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

The message here seems a bit misleading.

If I do Pkg.add("Foo") and I already have a directory Foo in Pkg.dir(), but Foo is not the Foo repo corresponding to the one in METADATA, I would expect an error rather than "Nothing to be done". Should we check that their origins match?

And if I already have a directory foo, an "unknown package Foo" error message is wrong.

At the very least, we should probably be doing:

ispath(pkg) || throw(PkgError("unknown package $pkg"))
Read.isgitrepo(pkg) || throw(PkgError("Cannot install $pkg because there is a pre-existing $pkg directory in $(pwd())"))

end
branch = Dir.getmetabranch()
outdated = with(GitRepo, "METADATA") do repo
if LibGit2.branch(repo) == branch
if LibGit2.isdiff(repo, "origin/$branch")
outdated = :yes
else
:no # user is doing something funky with METADATA
try
LibGit2.fetch(repo)
outdated = LibGit2.isdiff(repo, "origin/$branch") ? (:yes) : (:no)
end
end
else
:no # user is doing something funky with METADATA
end
end
if outdated != :no
Expand All @@ -79,7 +77,7 @@ add(pkg::AbstractString, vers::VersionNumber...) = add(pkg,VersionSet(vers...))

function rm(pkg::AbstractString)
edit(Reqs.rm,pkg) && return
ispath(pkg) || return info("Nothing to be done")
ispath_casesensitive(pkg) || return info("Nothing to be done")
info("Removing $pkg (unregistered)")
Write.remove(pkg)
end
Expand Down Expand Up @@ -113,7 +111,7 @@ function installed(pkg::AbstractString)
avail = Read.available(pkg)
if Read.isinstalled(pkg)
res = typemin(VersionNumber)
if ispath(joinpath(pkg,".git"))
if Read.isgitrepo(pkg)
Copy link
Member

Choose a reason for hiding this comment

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

Note that isinstalled already checks for a case-sensitive pkg, so this change is somewhat redundant. Doesn't hurt, I suppose.

LibGit2.with(GitRepo, pkg) do repo
res = Read.installed_version(pkg, repo, avail)
end
Expand Down Expand Up @@ -159,7 +157,7 @@ function status(io::IO, pkg::AbstractString, ver::VersionNumber, fix::Bool)
@printf io " - %-29s " pkg
fix || return println(io,ver)
@printf io "%-19s" ver
if ispath(pkg,".git")
if Read.isgitrepo(pkg)
prepo = GitRepo(pkg)
try
with(LibGit2.head(prepo)) do phead
Expand Down Expand Up @@ -222,7 +220,8 @@ function clone(url_or_pkg::AbstractString)
end

function checkout(pkg::AbstractString, branch::AbstractString, do_merge::Bool, do_pull::Bool)
ispath(pkg,".git") || throw(PkgError("$pkg is not a git repo"))
isdir_casesensitive(pkg) || throw(PkgError("$pkg is not installed"))
Read.isgitrepo(pkg) || throw(PkgError("$pkg is not a git repo"))
info("Checking out $pkg $branch...")
with(GitRepo, pkg) do r
LibGit2.transact(r) do repo
Expand All @@ -240,7 +239,7 @@ function checkout(pkg::AbstractString, branch::AbstractString, do_merge::Bool, d
end

function free(pkg::AbstractString)
ispath(pkg,".git") || throw(PkgError("$pkg is not a git repo"))
Read.isgitrepo(pkg) || throw(PkgError("$pkg is not a git repo"))
Read.isinstalled(pkg) || throw(PkgError("$pkg cannot be freed – not an installed package"))
avail = Read.available(pkg)
isempty(avail) && throw(PkgError("$pkg cannot be freed – not a registered package"))
Expand All @@ -267,7 +266,7 @@ end
function free(pkgs)
try
for pkg in pkgs
ispath(pkg,".git") || throw(PkgError("$pkg is not a git repo"))
Read.isgitrepo(pkg) || throw(PkgError("$pkg is not a git repo"))
Read.isinstalled(pkg) || throw(PkgError("$pkg cannot be freed – not an installed package"))
avail = Read.available(pkg)
isempty(avail) && throw(PkgError("$pkg cannot be freed – not a registered package"))
Expand All @@ -291,7 +290,8 @@ function free(pkgs)
end

function pin(pkg::AbstractString, head::AbstractString)
ispath(pkg,".git") || throw(PkgError("$pkg is not a git repo"))
isdir_casesensitive(pkg) || throw(PkgError("$pkg is not installed"))
Copy link
Member

Choose a reason for hiding this comment

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

Omit this isdir_casesensitive check since it is performed by isgitrepo now.

Read.isgitrepo(pkg) || throw(PkgError("$pkg is not a git repo"))
should_resolve = true
with(GitRepo, pkg) do repo
id = if isempty(head) # get HEAD commit
Expand Down Expand Up @@ -341,7 +341,7 @@ end
pin(pkg::AbstractString) = pin(pkg, "")

function pin(pkg::AbstractString, ver::VersionNumber)
ispath(pkg,".git") || throw(PkgError("$pkg is not a git repo"))
Read.isgitrepo(pkg) || throw(PkgError("$pkg is not a git repo"))
Read.isinstalled(pkg) || throw(PkgError("$pkg cannot be pinned – not an installed package"))
avail = Read.available(pkg)
isempty(avail) && throw(PkgError("$pkg cannot be pinned – not a registered package"))
Expand Down Expand Up @@ -411,7 +411,7 @@ function update(branch::AbstractString, upkgs::Set{String})
try
stopupdate = false
for (pkg,ver) in fixed
ispath(pkg,".git") || continue
Read.isgitrepo(pkg) || continue
pkg in dont_update && continue
with(GitRepo, pkg) do repo
if LibGit2.isattached(repo)
Expand Down Expand Up @@ -685,6 +685,10 @@ function test!(pkg::AbstractString,
errs::Vector{AbstractString},
nopkgs::Vector{AbstractString},
notests::Vector{AbstractString}; coverage::Bool=false)
if !isdir_casesensitive(pkg)
push!(nopkgs, pkg)
return false
end
reqs_path = abspath(pkg,"test","REQUIRE")
if isfile(reqs_path)
tests_require = Reqs.parse(reqs_path)
Expand All @@ -694,9 +698,7 @@ function test!(pkg::AbstractString,
end
end
test_path = abspath(pkg,"test","runtests.jl")
if !isdir(pkg)
push!(nopkgs, pkg)
elseif !isfile(test_path)
if !isfile(test_path)
push!(notests, pkg)
else
info("Testing $pkg")
Expand Down
1 change: 1 addition & 0 deletions base/pkg/pkg.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ type PkgError <: Exception
end
PkgError(msg::AbstractString) = PkgError(msg, Nullable{Exception}())
function Base.showerror(io::IO, pkgerr::PkgError)
print(io, "PkgError: ")
print(io, pkgerr.msg)
if !isnull(pkgerr.ex)
pkgex = get(pkgerr.ex)
Expand Down
48 changes: 32 additions & 16 deletions base/pkg/read.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,29 @@
module Read

import ...LibGit2, ..Cache, ..Reqs, ...Pkg.PkgError, ..Dir
using ..Types
using ..Types, Base.Filesystem

readstrip(path...) = strip(readstring(joinpath(path...)))
isgitrepo(pkg) =
isdir_casesensitive(pkg) && ispath(pkg, ".git")

url(pkg::AbstractString) = readstrip(Dir.path("METADATA"), pkg, "url")
sha1(pkg::AbstractString, ver::VersionNumber) = readstrip(Dir.path("METADATA"), pkg, "versions", string(ver), "sha1")
readstrip(path...) =
strip(readstring(joinpath(path...)))

url(pkg::AbstractString) =
readstrip(Dir.path("METADATA"), pkg, "url")

sha1(pkg::AbstractString, ver::VersionNumber) =
readstrip(Dir.path("METADATA"), pkg, "versions", string(ver), "sha1")

function available(names=readdir("METADATA"))
pkgs = Dict{String,Dict{VersionNumber,Available}}()
for pkg in names
isfile("METADATA", pkg, "url") || continue
versdir = joinpath("METADATA", pkg, "versions")
pkgdir = joinpath("METADATA", pkg)
if !isdir_casesensitive(pkgdir)
Copy link
Member

Choose a reason for hiding this comment

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

Is this really necessary? In cases where pkg is supplied by the METADATA, not by the user, can't we assume that the case is correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

names can be supplied by the user (Pkg.available("pkgname"). I could not measure any real performance difference in using the case_sensitive functions on OSX so why not use them defensively everywhere?

Copy link
Member

Choose a reason for hiding this comment

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

Okay.

continue
end
isfile(pkgdir, "url") || continue
versdir = joinpath(pkgdir, "versions")
isdir(versdir) || continue
for ver in readdir(versdir)
ismatch(Base.VERSION_REGEX, ver) || continue
Expand All @@ -28,13 +39,18 @@ function available(names=readdir("METADATA"))
end
return pkgs
end
available(pkg::AbstractString) = get(available([pkg]),pkg,Dict{VersionNumber,Available}())
available(pkg::AbstractString) =
get(available([pkg]),pkg,Dict{VersionNumber,Available}())

function latest(names=readdir("METADATA"))
pkgs = Dict{String,Available}()
for pkg in names
isfile("METADATA", pkg, "url") || continue
versdir = joinpath("METADATA", pkg, "versions")
pkgdir = joinpath("METADATA", pkg)
if !isdir_casesensitive(pkgdir)
continue
end
isfile(pkgdir, "url") || continue
versdir = joinpath(pkgdir, "versions")
isdir(versdir) || continue
pkgversions = VersionNumber[]
for ver in readdir(versdir)
Expand All @@ -53,12 +69,12 @@ function latest(names=readdir("METADATA"))
end

isinstalled(pkg::AbstractString) =
pkg != "METADATA" && pkg != "REQUIRE" && pkg[1] != '.' && isdir(pkg)
pkg != "METADATA" && pkg != "REQUIRE" && pkg[1] != '.' && isdir_casesensitive(pkg)

function isfixed(pkg::AbstractString, prepo::LibGit2.GitRepo, avail::Dict=available(pkg))
isinstalled(pkg) || throw(PkgError("$pkg is not an installed package."))
isfile("METADATA", pkg, "url") || return true
ispath(pkg, ".git") || return true
isgitrepo(pkg) || return true

LibGit2.isdirty(prepo) && return true
LibGit2.isattached(prepo) && return true
Expand Down Expand Up @@ -101,7 +117,7 @@ function isfixed(pkg::AbstractString, prepo::LibGit2.GitRepo, avail::Dict=availa
end

function ispinned(pkg::AbstractString)
ispath(pkg,".git") || return false
isgitrepo(pkg) || return false
LibGit2.with(LibGit2.GitRepo, pkg) do repo
return ispinned(repo)
end
Expand All @@ -115,7 +131,7 @@ function ispinned(prepo::LibGit2.GitRepo)
end

function installed_version(pkg::AbstractString, prepo::LibGit2.GitRepo, avail::Dict=available(pkg))
ispath(pkg,".git") || return typemin(VersionNumber)
isgitrepo(pkg) || return typemin(VersionNumber)

# get package repo head hash
local head
Expand Down Expand Up @@ -176,7 +192,7 @@ end

function requires_path(pkg::AbstractString, avail::Dict=available(pkg))
pkgreq = joinpath(pkg,"REQUIRE")
ispath(pkg,".git") || return pkgreq
isgitrepo(pkg) || return pkgreq
repo = LibGit2.GitRepo(pkg)
head = LibGit2.with(LibGit2.GitRepo, pkg) do repo
LibGit2.isdirty(repo, "REQUIRE") && return pkgreq
Expand All @@ -203,7 +219,7 @@ function installed(avail::Dict=available())
for pkg in readdir()
isinstalled(pkg) || continue
ap = get(avail,pkg,Dict{VersionNumber,Available}())
if ispath(pkg,".git")
if isgitrepo(pkg)
LibGit2.with(LibGit2.GitRepo, pkg) do repo
ver = installed_version(pkg, repo, ap)
fixed = isfixed(pkg, repo, ap)
Expand Down Expand Up @@ -238,7 +254,7 @@ function free(inst::Dict=installed(), dont_update::Set{String}=Set{String}())
end

function issue_url(pkg::AbstractString)
ispath(pkg,".git") || return ""
isgitrepo(pkg) || return ""
m = match(LibGit2.GITHUB_REGEX, url(pkg))
m === nothing && return ""
return "https://github.com/" * m.captures[1] * "/issues"
Expand Down
Loading