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

Jump to bottom

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

wants to merge 2 commits into from

Conversation

jakebolewski
Copy link
Member

Fixes the disparity between package names and module names w.r.t package name case sensitivity for Pkg operations as package names are module names.

Adds (unexported) {ispath,isdir,isfile}_casesensitive methods to the Base.Filesystem module

Also, removes the @async block in Pkg.add.

Closes #17747

cc @stevengj

@jakebolewski jakebolewski changed the title Case sensitive Pkg operations Case sensitive Pkg operations on case insensitive filesystems Aug 23, 2016
@jakebolewski jakebolewski mentioned this pull request Aug 23, 2016
Closed
@tkelman tkelman added the packages Package management and loading label Aug 23, 2016
stevengj
stevengj reviewed Aug 23, 2016
View reviewed changes
base/pkg/entry.jl
@@ -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.

stevengj and others added 2 commits August 26, 2016 11:38
@stevengj @jakebolewski
make Pkg operations case sensitive on case-insensitive filesystems (f…
f8dbc7c 
…ixes #17747)
@jakebolewski
Add additional Pkg case sen path comparisons and add tests
3262c17 
Additionally:

  *  print PkgError type in showerror
  *   get rid of async block in Pkg.add so that it no longer throws
      CompositeExceptions
@jakebolewski
Copy link
Member Author

jakebolewski commented Aug 26, 2016 •
edited
Loading

Addressed @stevengj's comments.

@tkelman is this backport worthy? This only restricts allowable behavior and the only "breaking" change is to no longer throw CompositeException in Pkg.add. I couldn't find any packages that relied on this behavior.

@tkelman
Copy link
Contributor

tkelman commented Aug 26, 2016

I'm against backporting this. Would have been a great addition a month ago before we feature froze.

@stevengj
Copy link
Member

-1 for backporting. I don't think leaving this as an 0.6-only feature is a problem; it's not like it affects code compatibility in any serious way. (In principle, it's a breaking change, but in practice it only catches typos.)

stevengj
stevengj reviewed Aug 26, 2016
View reviewed changes
base/pkg/entry.jl
@@ -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.

@stevengj
Copy link
Member

Bump.

@stevengj
Copy link
Member

Bump @jakebolewski; would be nice to get this finished?

This was referenced Nov 10, 2016
Merged
Closed
@kshyatt
Copy link
Contributor

kshyatt commented Dec 19, 2017

It seems like we can close this since it's quite old, has conflicts, and we're moving to Pkg3? Reopen if not!

@kshyatt kshyatt closed this Dec 19, 2017
@bkamins
Copy link
Member

bkamins commented Dec 19, 2017

@kshyatt A small question. Will moving to Pkg3 make using also case sensitive on Windows (this is the issue many of my students have on their first Julia session)?
What I mean is that on Windows we have the following behavior:

julia> using DATAFRAMES
WARNING: requiring "DATAFRAMES" in module "Main" did not define a corresponding module.

julia> DataFrames.DataFrame()
0×0 DataFrames.DataFrame

julia> DataFrame()
ERROR: UndefVarError: DataFrame not defined

which is really confusing for newcomers.

@StefanKarpinski
Copy link
Member

I do think that this remains at least partially applicable.

@DilumAluthge DilumAluthge deleted the jcb/pkg_casesensitive branch March 25, 2021 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
packages Package management and loading
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jakebolewski @tkelman @stevengj @kshyatt @bkamins @StefanKarpinski