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.

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

using X operates relative to the working directory #9079

Closed
MikeInnes opened this issue Nov 20, 2014 · 53 comments
Closed

using X operates relative to the working directory #9079

MikeInnes opened this issue Nov 20, 2014 · 53 comments
Assignees
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@MikeInnes
Copy link
Member

i.e. using X will load X.jl and the module X, but only from the current working directory, as opposed to the current file path as include does.

@MikeInnes MikeInnes changed the title using X operatates relative to the working directory using X operates relative to the working directory Nov 20, 2014
@kmsquire
Copy link
Member

I realize this was talked about elsewhere, but can you add a pointer and some more context?

@MikeInnes
Copy link
Member Author

If it's been discussed before then I missed that. It just came up for a new user on the Julia-LT chat room – they were trying out some Forio tutorial code which worked when using Julia from the REPL, but not in Juno, causing some confusion.

We could make this work, but since it doesn't seem to be particularly idiomatic (and is easily replaced by include("x.jl"); using X) it might be best to disable it completely.

@garborg
Copy link
Contributor

garborg commented Nov 20, 2014

Only place I've seen it mentioned: confusion when a test file name is a case-insensitive match to the name of a module/package that is used in a test file or required by a package used in a test file: #9007, #7256

@garborg
Copy link
Contributor

garborg commented Nov 20, 2014

If there's no downside, it seems desirable to be able to organize tests without thinking "my package uses Images, so I better not name this file 'color.jl'" -- bad example, but sometimes that's exactly how you'd want to organize your tests. It's also just a little confusing, and if there are good reasons to replace Color, I'd imagine it would be better for an already-loaded Color module to be picked up (right now, 'test/color.jl' is favored over an already-loaded Color module).

@shashi
Copy link
Contributor

shashi commented Jul 14, 2015

This wasted a few hours of time debugging for @jiahao @alanedelman and me. And some more for @rohitvarkey yesterday. It would be nice if someone with a mac looks into fixing this.

@jiahao jiahao added the bug Indicates an unexpected problem or unintended behavior label Jul 14, 2015
@timholy
Copy link
Member

timholy commented Jul 14, 2015

This issue report is lacking any kind of test case, making it hard to (1) know what you're concerned about and (2) test it on different platforms (since there seems to be some suspicion it's OSX-specific).

Just to show how ambiguous this report is, here's a test-case that appears to fit the wording used in the issue report but which comes to the opposite conclusion:

julia> pwd()
"/home/tim/Documents"

julia> LOAD_PATH
3-element Array{Union{UTF8String,ASCIIString},1}:
 "/home/tim/src/julia/usr/local/share/julia/site/v0.4"
 "/home/tim/src/julia/usr/share/julia/site/v0.4"      
 "/home/tim/juliafunc"                                                                                                                                                             

julia> edit("/home/tim/juliafunc/TestModule.jl")  # create a module that just prints "blah" when loaded

julia> using TestModule
blah

Moreover, include("TestModule.jl") fails (from this directory). I suspect you're referring to the machinations that get performed on the source path, but an explicit demo would be priceless.

@MikeInnes
Copy link
Member Author

~/test/
  test.jl -> "using Foo"
  Foo.jl  -> "module Foo println(:success) end"
shell> cd test
julia> include("test.jl")
success

I think it's reasonably intuitive that this works, even though Foo isn't on the load path. It makes a lot of sense to look for modules in the directory of the file.

julia> include("test/test.jl")
ERROR: could not open file C:\Users\mikej_000\Downloads\juno-windows64\test.jl
 in include at boot.jl:245
 in include_from_node1 at loading.jl:128

But then it's completely surprising that this doesn't work.

@ScottPJones
Copy link
Contributor

So, should it look first in the current working directory, or in the directory where the file containing the using was loaded? (I can see pros and cons to both approaches, but I think @one-more-minute is correct, that it really should be looking in the directory where the file containing the using was loaded, probably before the load path).

@shashi
Copy link
Contributor

shashi commented Jul 14, 2015

Here are the steps I took to reproduce this bug:

On Julia Version 0.3.10 (2015-06-24 13:54 UTC)

julia> Pkg.generate("Temp", "MIT")
INFO: Initializing Temp repo: /Users/rohitvarkey/.julia/v0.3/Temp
INFO: Generating LICENSE.md
INFO: Generating README.md
INFO: Generating src/Temp.jl
INFO: Generating test/runtests.jl
INFO: Generating .travis.yml
INFO: Generating .gitignore
INFO: Committing Temp generated files
julia> edit("Temp.jl")
# this is the module definition
 module Temp
  println("Loading module")
  end # module

Create /tmp/temp.jl with the following contents:

#temp.jl:
  println("Loading file...")

Run this in a julia REPL under /tmp/

shell> cd /tmp/

julia> using Temp
Loading file...
Warning: requiring "Temp" did not define a corresponding module.

But if I run using Temp from a REPL under ~/ where there is no file named temp.jl julia correctly loads the module.

julia> using Temp
Loading module

@JeffBezanson
Copy link
Member

That looks like ok behavior to me; it's just searching the current directory first.

My understanding of the bug is this: if file a.jl does include("b.jl"), we first look for b.jl in the same directory as a.jl. But if a.jl does using b, we look in the standard paths but not necessarily in the same directory as a.jl.

@MikeInnes
Copy link
Member Author

But crucially, using b also looks for b.jl in pwd(). Maybe there's a valid use case for that behaviour, but none spring to mind, and it's caused confusion for more than one person.

It doesn't seem hugely unlikely that you might have e.g. a color.jl file lying around, and if you happen to launch Julia from the same directory, packages that do using Color are going to break in a way that's really hard to debug.

Seems much clearer for people to explicitly write include(joinpath(pwd(), "b.jl")) if they want that kind of behaviour.

@jakebolewski
Copy link
Member

using B looking in the pwd() is necessary if you write scripts that contain modules not in Pkg.dir(). This is really handy for any kind of development that is not simply writing libraries.

@ScottPJones
Copy link
Contributor

Could including pwd() by default for using b cause any security issues?

@shashi
Copy link
Contributor

shashi commented Jul 14, 2015

Oh, I guess then this is more surprising on OS X, because filenames are
case insensitive.

And this is why I can't have a file called examples/compose.jl with an
example that uses the Compose.jl package. Is that OK though..?

On Tue, Jul 14, 2015, 8:49 PM Jeff Bezanson [email protected]
wrote:

That looks like ok behavior to me; it's just searching the current
directory first.

My understanding of the bug is this: if file a.jl does include("b.jl"),
we first look for b.jl in the same directory as a.jl. But if a.jl does using
b, we look in the standard paths but not necessarily in the same
directory as a.jl.


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

@MikeInnes
Copy link
Member Author

@jakebolewski But why would you want to look in pwd() as opposed to the current source file's directory? All that's going to mean is that your script breaks if you run julia scripts/script.jl as opposed to julia script.jl.

In other words – what value do you expect pwd() to take, and when is it reliable?

@aviks
Copy link
Member

aviks commented Jul 14, 2015

Is that OK though

Well, in linux, you wouldnt be able to have a file called "examples/Compose.jl" for the same reason. OSX does have a case insensitive file system by default, so that's the behaviour you will get. I don't think Julia should compensate for that. The productivity loss due to issues like this, lay on the hands of Apple :)

@shashi
Copy link
Contributor

shashi commented Jul 15, 2015

This seems fine to me now.

by default

you mean there is a mode where it's not case sensitive? :o

The productivity loss due to issues like this, lay on the hands of Apple

I can settle for that.

@jiahao
Copy link
Member

jiahao commented Jul 15, 2015

you mean there is a mode where it's not case sensitive? :o

You can format a HFS+ partition to be case sensitive, but it is not the default.

@carnaval
Copy link
Contributor

You can format a HFS+ partition to be case sensitive, but it is not the default.

that would make a good, although sneaky, make install step

@johnmyleswhite
Copy link
Member

Many OS X apps (for a long time everything by Adobe) can't handle case-sensitive file systems.

@JeffBezanson JeffBezanson self-assigned this Jul 18, 2015
@JeffBezanson
Copy link
Member

This should definitely be changed. The exact behavior of include is that it only looks relative to the current file, unless there is no current file (i.e. you're at the prompt), in which case it uses the CWD. I think using should work the same way (except of course that it also uses the search path).

@jiahao
Copy link
Member

jiahao commented Jul 22, 2015

This change broke Gadfly (possibly on OSX only, due to case insensitivity).

Gadfly has a source file src/color.jl. In the Gadfly module defined in src/Gadfly.jl, using Color comes before include("color.jl"). Now, using Color now loads the local source file instead of the Color package, causing Gadfly to not find color types defined in Color.jl.

Rolling back to the commit prior to 189a043 restores working Gadfly.

@tkelman
Copy link
Contributor

tkelman commented Jul 26, 2015

No, that shouldn't be necessary. It's technically possible to change a registry setting and use Cygwin in a case-sensitive mode on Windows if you want, but very few people do that. Case-insensitivity is stupid and annoying, but until it goes away as a default setting we need to test for it and be robust to it.

As one example, having multiple files with names that differ only in case in the same folder of a package can cause a package repo to always be dirty when checked out in git, so that package will get stuck in a state where it won't update correctly from then on. This kind of thing is a portability red flag that we need to keep a close eye out for.

@aviks
Copy link
Member

aviks commented Jul 26, 2015

I don't think we should be focussing too much about OSX and case sensitive file systems when analysing this issue.

Yes, bindeps.jl had to be changed to bindeps_integration.jl on OSX, while it works on Linux. However, even on linux, a package now cannot have Bindeps.jl, from what I understand. Is that OK? If so, this means all package files are in a global namespace. I think that's a pretty big deal, and needs documentation.

@ScottPJones
Copy link
Contributor

@tkelman We aren't using Windows at all, using the case-sensitive sparsebundle on the Mac is simply to avoid issues between the host OS (where we do our editting, etc.) and our Linux VMs.
@aviks Yes, you are right, this problem is a big deal, it just showed up easier on Mac and Windows due to default case-insensitivity.

@tkelman
Copy link
Contributor

tkelman commented Jul 26, 2015

@ScottPJones I get the impression when you say "we" you're talking about your company and coworkers. That is completely and utterly irrelevant here. When I say "we" I'm talking about the Julia community as a whole, across all platforms that are currently supported.

Documentation isn't an acceptable fix for this, we need to walk back on this change. We certainly can't keep things as they are right now for a release.

@ScottPJones
Copy link
Contributor

@tkelman I felt it was relevant, as an example of a way of avoid at least the extra problems caused by using case-insensitive file systems. As I said to aviks, I do think this is a big deal, and needs to be fixed for all cases, not just partially patch up the issue for the default on OS X and Windows.

@ScottPJones
Copy link
Contributor

Also, I don't think that just removing the cwd or the source file location really fixes this either.
I think something would need to detect the looping, and give an error, because one can cause the exact same problem by adding something with a conflict like this to LOAD_PATH, right?

@JeffBezanson
Copy link
Member

It's unintuitive to me that this would be characterized as putting all package files in a single global namespace. Technically, it does the exact opposite of that --- looking in the source file directory means all packages can have a file with the same name without conflicts.

Anyway, maybe the solution is #4600: have using X look only in LOAD_PATH, and have using .X look in the source file directory.

@aviks
Copy link
Member

aviks commented Jul 26, 2015

It's unintuitive to me that this would be characterized as putting all package files in a single global namespace

well, yes, maybe that's too strong a term technically, but now you cannot have a local file that is named the same as a module+file anywhere in LOAD_PATH, as all the issues linked above show.

The previous behaviour did have its issues, and the fix seemed logical in making things more consistent. But somehow the earlier behaviour seemed much more intuitive. Am I just rationalising learnt behaviour?

@JeffBezanson
Copy link
Member

I'm fairly certain the reason the old behavior worked seemed to work is that people tend not to have files called color.jl or bindeps.jl in their current working directory (e.g. /home/jeff/project1), but it's common for packages to have such files for connecting with other packages. If we changed this to only search LOAD_PATH, I suspect nobody would notice.

@garborg
Copy link
Contributor

garborg commented Jul 27, 2015

As a new user, I didn't think the old behavior worked (on OSX): #7256. At least not when working on packages (as opposed to general projects).

@shashi
Copy link
Contributor

shashi commented Jul 27, 2015

Anyway, maybe the solution is #4600: have using X look only in LOAD_PATH, and have using .X look in the source file directory.

+1

@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2015

So #12340 didn't fix the the package filename collisions causing freezes for .jl files, ref JuliaIO/JLD.jl#8 (comment). We still need to do something about this - IMO, before considering a release.

@jdlangs
Copy link
Contributor

jdlangs commented Aug 3, 2015

Would it fix the issue to have the source directory as a fallback to LOAD_PATH instead of the reverse? This seems to be how Python resolves conflicts, picking the system module over the local one with the same name.

@tkelman
Copy link
Contributor

tkelman commented Aug 3, 2015

I wouldn't mind syntactic distinction between the two with the leading . - that way the intent is always clear.

@jdlangs
Copy link
Contributor

jdlangs commented Aug 3, 2015

I think there's value in allowing where to find external code to be controlled by the system configuration (build tool arguments, environment variables, etc...) and not in the syntax of the actual code text.

But it might be nice to have a parallel using/import construct that only checks the local directory so that include isn't the only option.

@JeffBezanson
Copy link
Member

I think the idea is for using .X to only check the local directory.

@ScottPJones
Copy link
Contributor

Meaning the local directory the current module was found in, unless at REPL?

@StefanKarpinski
Copy link
Member

Yes. In the REPL the current directory is where you would look.

@ScottPJones
Copy link
Contributor

Sounds very good to me.

@Keno
Copy link
Member

Keno commented Aug 16, 2015

After using this for a while, I find using looking in the source directory extremely annoying. +1 to just have it look in LOAD_PATH and do something else for source local includes.

@StefanKarpinski
Copy link
Member

Plan:

@StefanKarpinski StefanKarpinski added this to the 0.4.0 milestone Aug 19, 2015
@StefanKarpinski StefanKarpinski added priority This should be addressed urgently and removed priority This should be addressed urgently labels Aug 19, 2015
@StefanKarpinski StefanKarpinski removed this from the 0.4.0 milestone Aug 19, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests