Make require case-sensitive on all platforms #13542

malmaud · 2015-10-11T06:11:51Z

Resolves #5789.

tkelman · 2015-10-11T07:14:05Z

I'd expect readdir to be really slow here.

malmaud · 2015-10-11T17:45:29Z

I don't think there's any other reliable solution. I wonder how much of a concern it is in practice - the check can be disabled on linux, which is the OS where it's most likely there will remote volumes where readdir's performance could be particularly problematic.

If the folders are local, then I'm sceptical the cost of ~20 readdir's on package src directories is meaningful compared the total runtime of importing a package.

malmaud · 2015-10-11T20:40:41Z

@simonster suggested using realpath, which will work on Linux on OS X. Still not sure there is a great solution for windows - the best I could find is http:https://stackoverflow.com/questions/4763117/how-can-i-obtain-the-case-sensitive-path-on-windows, but that doesn't seem great since it will randomly fail if the underlying volume has short path names disabled.

malmaud · 2015-10-12T01:38:30Z

Simon correctly points out that the realpath solution won't work with symbolic links.

simonster · 2015-10-12T01:42:27Z

But you could still use it as long as islink returns false, and fall back to the slow path if islink returns true (shouldn't be very common).

malmaud · 2015-10-12T03:27:17Z

OK, the only case where the pr falls back to listing a directory is if the file is a symbolic link on OS X. I think I have a working solution for Windows - at least it works on my Windows 7 x64 box.

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man2/getattrlist.2.html might help with OS X.

simonster · 2015-10-12T03:31:35Z

base/loading.jl

+@osx_only function isfile_casesensitive(path)
+ isfile(path) || return false
+ islink(path) && return isfile_casesensitive_slow(path)
+ realpath(path) == path


Unless you can find a better solution, I think this should check basename(realpath(path)) == basename(path) in case path is not a symlink but is in a symlinked directory

StefanKarpinski · 2015-10-12T05:31:39Z

Excellent – thanks for tackling this.

tkelman · 2015-10-12T05:41:32Z

needs a test

malmaud · 2015-10-12T05:51:25Z

Good news - I've got working C code for getting the canonical capitalization in OS X. Just a matter of wrapping it in Julia now.

malmaud · 2015-10-12T07:12:15Z

OK, all the functionality is here. I'll clean it up and tests and then it should be g2g.

stevengj · 2015-10-12T14:55:33Z

base/loading.jl

@@ -2,6 +2,52 @@

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

+# Generic case-sensitive version of isfile
+function isfile_casesensitive_slow(path)


Seems like it would be cleaner to do

if OS_NAME == :Windows function isfile_casesensitive(path) ... end elseif OS_NAME == :Linux ... else ... end

stevengj · 2015-10-12T15:20:44Z

Also fixes #9007.

stevengj · 2015-10-12T15:30:29Z

See also how Python 2.7 does it.

malmaud · 2015-10-12T15:31:50Z

Looks like it's walking the directory tree.

stevengj · 2015-10-12T15:32:34Z

Yes, looks like it. Of course, they had to support OS/2 and other ancient systems, so maybe the things you are doing were not available at some point.

stevengj · 2015-10-12T15:37:02Z

base/loading.jl

+ buf_size *= 2
+ continue
+ end
+ canon_path = bytestring(pointer(buf, 13))


Couldn't you just do canon_path = UTF8String(buf) in order to avoid making a copy?

The path is stored at an offset from the beginning of the buffer, so I had to use a somewhat inelegant workaround.

stevengj · 2015-10-12T18:46:19Z

I'm not sure this is handling Unicode normalization properly.

Suppose you have a file named "ô". There are two logically equivalent ways to encode this string in Unicode, given by normalize_string("ô", :NFC) (= one codepoint U+00f4) and normalize_string("ô", :NFD) (= two codepoints U+006f and U+0302).

On MacOS, isfile returns true for either encoding. Internally, HFS+ converts all names to NFD, and presumably this is what getattrlist returns. That means that isfile_casesensitive(normalize_string("ô", :NFC)) will fail even though the file exists. Solution: call nfd(path) before comparing, where nfd(s::AbstractString) = isascii(s) ? bytestring(s) : normalize_string(s, :NFD).

On Windows, I'm not 100% sure of the situation. The documentation says There is no need to perform any Unicode normalization on path and file name strings for use by the Windows file I/O API functions because the file system treats path and file names as an opaque sequence of WCHARs. This makes me think that normalize_string("ô", :NFC) and normalize_string("ô", :NFD) are distinct filenames on Windows, but it would be good to verify this (i.e. just try creating two files with these names in the same directory and see whether they co-exist).

(On Linux and probably most other Unix-like systems, the NFC and NFD versions are distinct filenames.)

johansigfrids · 2015-10-12T20:04:19Z

Yes, I can confirm normalize_string("ô", :NFC) and normalize_string("ô", :NFD) are distinct files on Windows. Windows file system is Unicode (and UTF-16) mostly by convention, in practice its is just a Vector{UInt16} for which a best effort is made to display as UTF-16.

malmaud · 2015-10-12T20:21:15Z

Great catch @stevengj .

nalimilan · 2015-10-12T21:09:04Z

And what happens on OS X when a different filesystem from HFS+ is used? Should we even care about that scenario?

malmaud · 2015-10-12T21:21:43Z

You're right, that's a real concern. If a file is stored on a mounted non-hfs+ directory, has a unicode character, and isn't NFD-normalized, it would be invisible to require. Maybe we should stat the file to get the filesystem its mounted on and only do the utf normalization if its on hfs+.

malmaud · 2015-10-12T21:25:53Z

The same issue applies to linux: What if the file happens to be on a case-insensitive filesystem? Then loading.jl:9 is wrong. Although at least it's not as serious, since it would be no more wrong than the current behavior of require.

malmaud · 2015-10-13T01:09:54Z

Alright, I think I worked around the OS X non-HFS+ concern wrt utf normalization.

stevengj · 2015-10-13T02:08:19Z

base/loading.jl

+ if normed_path == path
+ false
+ else
+ isfile_casesensitive(normed_path)


Why not just:

isascii(path_basename) && return false # shortcut for common case of ASCII paths normed_basename = normalize_string(path_basename, :NFD) return normed_basename.data == casepreserved_basename

stevengj · 2015-10-13T03:27:42Z

LGTM once Travis is green and the commits are squashed.

stevengj · 2015-10-13T03:30:22Z

test/loading.jl

@@ -10,3 +10,17 @@ thefname = "the fname!//\\&\0\1*"
 @test include_string("Base.source_path()", thefname) == Base.source_path()
 @test basename(@__FILE__) == "loading.jl"
 @test isabspath(@__FILE__)
+
+let true_filename = "cAsEtEsT.jl", lowered_filename="casetest.jl"


Maybe add a comment here:

# Issue #5789 and PR #13542:

malmaud · 2015-10-13T14:38:31Z

OK, squashed and tests are green.

Make require case-sensitive on all platforms

stevengj · 2015-10-13T14:53:10Z

Thanks, @malmaud, for pushing this through!

malmaud · 2015-10-13T14:54:40Z

Thanks for the reviewing!
On Tue, Oct 13, 2015 at 10:53 AM Steven G. Johnson [email protected]
wrote:

Thanks, @malmaud https://github.com/malmaud, for pushing this through!

—
Reply to this email directly or view it on GitHub
#13542 (comment).

stevengj · 2015-10-13T14:57:31Z

Can you submit a quick PR for realpath before we forget?

stevengj · 2016-11-09T23:43:05Z

base/loading.jl

+ # GetLongPathName Win32 function returns the case-preserved filename on NTFS.
+ function isfile_casesensitive(path)
+ isfile(path) || return false # Fail fast
+ longpath(path) == path


Possibly this should be basename(longpath(path)) == basename(path)? Two reasons:

On MacOS, we only check that the basename matches, so this is inconsistent.

The intent of this was that import Foo would check for Foo in a case-sensitive way. But this does more than that, and checks the whole path. This means that users LOAD_PATH and HOME variables becomes case-sensitive, for example, which is unexpected on case-insensitive systems, and has led to at least one confused user on discourse.

simonster reviewed Oct 12, 2015
View reviewed changes

stevengj reviewed Oct 12, 2015
View reviewed changes

stevengj reviewed Oct 13, 2015
View reviewed changes

Make require case-sensitive on all platforms

b78f560

malmaud force-pushed the jmm/case_sensitive_loading branch from d61579f to b78f560 Compare October 13, 2015 04:06

stevengj added a commit that referenced this pull request Oct 13, 2015

Merge pull request #13542 from JuliaLang/jmm/case_sensitive_loading

b6c0d95

Make require case-sensitive on all platforms

stevengj merged commit b6c0d95 into master Oct 13, 2015

stevengj deleted the jmm/case_sensitive_loading branch October 13, 2015 14:52

stevengj mentioned this pull request Oct 13, 2015

Documentation for require() should be updated #13579

Closed

malmaud mentioned this pull request Oct 13, 2015

Fix realpath on Windows #13582

Closed

stevengj added a commit that referenced this pull request Oct 13, 2015

NEWS for #13542

8adbf7a

jakebolewski mentioned this pull request Oct 16, 2015

Case Issue with Pkg Manager #9094

Closed

This was referenced Nov 4, 2015

Package names should be case sensitive #13873

Closed

freeze due to name collision in module loading on case-insensitive filesystems #9007

Closed

Make Pkg operations case sensitive on all platforms? #13898

Closed

andrewadare mentioned this pull request Mar 20, 2016

ERROR: LoadError: ArgumentError: LibSerialPort not found in path JuliaIO/LibSerialPort.jl#1

Closed

yuyichao mentioned this pull request Jul 2, 2016

Mention require's case-sensitivity in documentation #17249

Merged

stevengj mentioned this pull request Aug 1, 2016

Pkg operations should be case-sensitive on case-insensitive filesystems #17747

Closed

stevengj reviewed Nov 9, 2016

View reviewed changes

stevengj mentioned this pull request Nov 10, 2016

make isfile_casesensitive only check case of the basename on Windows #19291

Merged

stevengj mentioned this pull request Jan 4, 2019

inconsistent realpath on Windows #30588

Closed

This was referenced May 2, 2019

"using" module name with wrong case in case-insensitive filesystem tonyhffong/Lint.jl#72

Open

Module loading case-sensitivity windows inconsistency #31913

Open

Make require case-sensitive on all platforms #13542

Make require case-sensitive on all platforms #13542

Conversation

malmaud commented Oct 11, 2015

tkelman commented Oct 11, 2015

malmaud commented Oct 11, 2015

malmaud commented Oct 11, 2015

malmaud commented Oct 12, 2015

simonster commented Oct 12, 2015

malmaud commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Oct 12, 2015

tkelman commented Oct 12, 2015

malmaud commented Oct 12, 2015

malmaud commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Oct 12, 2015

stevengj commented Oct 12, 2015

malmaud commented Oct 12, 2015

stevengj commented Oct 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Oct 12, 2015

johansigfrids commented Oct 12, 2015

malmaud commented Oct 12, 2015

nalimilan commented Oct 12, 2015

malmaud commented Oct 12, 2015

malmaud commented Oct 12, 2015

malmaud commented Oct 13, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Oct 13, 2015

Choose a reason for hiding this comment

malmaud commented Oct 13, 2015

stevengj commented Oct 13, 2015

malmaud commented Oct 13, 2015

stevengj commented Oct 13, 2015

Choose a reason for hiding this comment