Issue1661700

See the example below. Note that d drive on my WinXP is a CD drive and I don't have r drive at all.  

C:\>d:
The device is not ready.

C:\>r:
The system cannot find the drive specified.

C:\>jython
Jython 2.2b1 on java1.5.0_10 (JIT: null)
Type "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.abspath('foo')
'C:\\foo'
>>> os.path.abspath('r:\\foo')
'R:\\foo'
>>> os.path.abspath('d:\\foo')
Traceback (innermost last):
  File "<console>", line 1, in ?
  File "C:\jython2.2b1\Lib\javapath.py", line 240, in abspath
        at java.io.WinNTFileSystem.canonicalize0(Native Method)
        at java.io.Win32FileSystem.canonicalize(Unknown Source)
        at java.io.File.getCanonicalPath(Unknown Source)
        at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
        at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source
        at java.lang.reflect.Method.invoke(Unknown Source)

java.io.IOException: java.io.IOException: The device is not ready
>>>

C:\>python
Python 2.5 (r25:51908, Sep 19 2006, 09:52:17) [MSC v.1310 32 bit (Intel)] on win
32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> os.path.abspath('foo')
'C:\\foo'
>>> os.path.abspath('r:\\foo')
'r:\\foo'
>>> os.path.abspath('d:\\foo')
'd:\\foo'

Probably the best solution for this is catching possible IOException from getCanonicalPath and using getAbsolutePath in that case. Jython 2.2alpha1 used getAbsolutePath and this problem doesn't appear there. The implementation was changed to fix http://jython.org/bugs/1423047

Bad news: Related problems also in realpath and samefile. 

Good news: I have a patch ready but want to write some tests for it before submitting. Should get those done tomorrow.

No more exceptions if you apply the patch I provided to #1648449 (os.path.normcase broken in Windows).

Problem is os.path is really either dospath, macpath, ntpath, posixpath -- in CPython.
In Jython, ...\Lib\javaos.py was always unconditionally loading javapath (a Jython creation):
    import javapath as path
and javapath's isabs() test is very different from ntpath -- javapath calls File(path).isAbsolute() which appears to touch the file system whereas ntpath's isabs() just calls splitdrive() and does a simple string test.

In short, javapath does a 'live' test dependent on the state of the underlying OS at that moment, whereas ntpath does a definitional test (does the path meet Windows' criteria for an absolute pathname).

NOTE: isabs() is a definitional test in all CPython flavors, independent of the current state of the underlying OS. Check dospath, macpath, ntpath and posixpath. Only javapath relies on the current state of the OS.

Needless to say, I prefer my patch to load ${os}path.py :-)

A patch fixing this and http://jython.org/bugs/1662689 available at http://jython.org/patches/1716709

This patch simply fixes these bugs and doesn't address the larger issue that the patch from hsk0 targets. Personally I don't care how these bugs are fixed -- just hope they will be. =) 

While I agree with hsk0's arguments for his patch in http://jython.org/bugs/1648449 in principle, the platform specific path modules depend on things existing in os.stat that Jython doesn't provide and javapath works around.  Using the Python specific modules would provide a better experience if they worked properly, but they won't until they have the same workarounds that javapath has.  Getting a more functional Java specific path module today is better than waiting for a completely functional platform specific path implementation to show up at some unknown point in the future.

That's the long way of saying the patch was applied in r3225.

I understand your concerns re: incomplete/broken platform-specific modules, and share them.

I've reviewed a good chunk of these differences. Here's what I see:
1. Most of the platform specific code looks good as-is
2. Some of the platform-specific code doesn't work as-is; for instance, the os.stat() calls in ntpath.py are a problem
3. The latest javapath.py has problems, explicit and implicit

I think #2 can be fixed with limited effort, or if not 100% then no worse than the current javapath.py. Certainly more complete and desired than #3.

So far, everything I've found in the platform-specific .py's that looks problematic seems fixable (derived in no small part from the work in javapath.py). I'm partially done and it looks promising so far. It looks like this update will entail:
* Adding or Modifying 6 .py files in CPythonLib (e.g. replacing os.stat() calls in ntpath.py)
* Modifying javaos.py
* Probably also modifying javapath.py (I have some thoughts, but haven't gotten there yet)
* Test cases

I'll post my patch shortly, once I get the bits a bit further along.

Besides os.stat, if you know any specific tips or gotchas to pay close attention to, do share.


I just have one question: In javapath.py, why is _tostr() used?

From what I can tell, it acts as a blocker only allowing strings past, typically used on parameters in javapath fuctions; non-string parameters kick a pretty exception message.

Why?

Reviewing the CPython platform-specific code, parameters are just used, expecting 'the right thing'. For instance, macpath.py's isabs(s) is a 1 liner:

    return ':' in s and s[0] != ':'

So the parameter has to support the 'in' and index operators or CPython would throw an exception, but they don't specifically require the parameter to be a basestring. Compare with the javapath.py implementation

def isabs(path):
    path = _tostr(path, "isabs")
    return File(path).isAbsolute()

It seems like javapath requires string parameters whereas the CPython bits accept any string-like type.

The latter seems more Pythonic to me, but I'm not clear on the history behind all this code so I'm wondering - why?

Is javapath's _tostr() behavior important for some subtle reason I'm not spotting?
Desirable for some reason I'm not picking up?
A historical necessity or accident, but which is now irrelevant (or even harmful)?

Wondering if, as I fixup the platform-specific *path.py's, if _tostr()-like behavior is something I need to worry about.

It looks like _tostr was added in r2009(http://fisheye3.cenqua.com/changelog/jython?cs=2009) to address http://jython.org/bugs/495602  From that, I don't think we really want to restrict things to basestring only, but actually want to ensure that None doesn't get passed to java.io.File.  

As for replacing javapath.py with CPython's platform specific modules, I'm all for it, but I think it's too late in the release process for 2.2 to make the change.  I'm planning on making the first release candidate next weekend.  I don't think we have enough time to try this out on all of the platforms to make sure it's not broken in some way compared to the existing system.  Please continue working on it and submit a patch though.  I'll happily put it in the 2.3 branch.

I totally agree with Charlie that platform specific os.path modules would be a great addition to 2.3 (or whatever the next version number will be). Changing os.path that much for 2.2 would be quite risky especially because it works (at least mostly).

One sure thing is that if/when the change is done some test cases are needed. Without automated tests that are run on all the platforms (via buildbots) it's really hard to get everything working. It would probably be best to write tests already for the current implementation so that when it's changed possible regressions are noticed. Unfortunately writing unit tests for functionality that's this closely tied to filesystem and behaves differently on different platforms is not trivial.

Several points here:


>writing unit tests for functionality that's this closely tied to filesystem
>and behaves differently on different platforms is not trivial
Aha. You see the inherent difficulties :->

Check the Python source distribution.
Python 2.2.3 and later  versions include ...\Lib\test\test_ntpath.py, test_posixpath.py and test_macpath.py, with at least reasonable tests for join, normpath , split, splitunc, splitdrive, isabs, abspath and commonprefix. test_os.path contains additional tests, for temp files and stat, though we know the latter's rather pointless right now. Those are more involved too, based on unittest. The others, test_*path.py, just 'from test_support import verbose, TestFailed', and neither really matters -- verbose is just =0 or =1 tested at the end if a successful run should be silent or say "No errors"; TestFailed is just a convenient exception used in test_*path.py to raise an exception at the end if an error's encountered.

In short, test_*path.py should be trivially liftable from CPython, with trivial (irrelevant) changes. That at least buys us a significant amount of testing 'for free'. I'll look thru the rest of CPython's test cases, see if there's more Jython could benefit from.


>It looks like _tostr was added...don't think we really want to restrict things
>to basestring only, but actually want to ensure that None doesn't get
>passed to java.io.File.
Sigh. With the URL, yes, I concur. It's a shame _tostr() was injected.
But one wonders - why?
Why even check?
CPython doesn't.
ntpath.py implements dirname() as just return split(p)[0], and you get the usual exception

Python 2.3.5 (#62, Feb  8 2005, 16:23:02) [MSC v.1200 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os.path
>>> os.path.dirname(None)
Traceback (most recent call last):
  File "<stdin>", line 1, in ?
  File "E:\Python23\lib\ntpath.py", line 206, in dirname
    return split(p)[0]
  File "E:\Python23\lib\ntpath.py", line 163, in split
    d, p = splitdrive(p)
  File "E:\Python23\lib\ntpath.py", line 118, in splitdrive
    if p[1:2] == ':':
TypeError: unsubscriptable object
>>>

However, CPython is rather inconsistent. For instance, commonprefix()'s first line is
    if not m: return ''
aka None testing. However, that appeasr to be unique -- nothing else in the *path.py's seem to explicitly test; some coincidentally do, e.g. exists() does an os.stat() in a try block returning 0 if anything's raised, but that seems more coincidental than intentional.

I'll submit a patch to lift the _tostr() restriction, but I can see 2 options:
* Change _tostr() to _nonnull(), replacing if isinstance(s,basestring) with if s!=None
* Remove _tostr(), i.e. revert r2009 and mark 495602 as 'Work as Designed'
Personally, I prefer the latter. It's more 'standard' <sic>, it's less work, and special-casing one small piece of the library for invalid None parameters seems, overall, more harm than good. Sure, NPE's are no fun - but consistency is a powerful force. After all, if we didn't like consistency so, we'd just use Perl ;-)

There's a 3rd option, that may be desirable
* Change _tostr() to _nonnull(), but make the test if s!=None or not java.lang.System.getProperty('jython.debug.NPE')
Use a config option (jython.debug.NPE=<boolean>, default=False) to enable 'pretty NPE exceptions'. In this case, _nonnull() could be a broadly used function, anyplace None is not valid it could be trapped and kick a pretty exception instead of what Jython routinely kicks now
[F:\jython2.2b2]jython.bat -c "import java.io.File; java.io.File(None)"
Traceback (innermost last):
  File "<string>", line 1, in ?
        at java.io.File.<init>(Unknown Source)
        at sun.reflect.GeneratedConstructorAccessor1.newInstance(Unknown Source)
        at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(Unknown Source)
        at java.lang.reflect.Constructor.newInstance(Unknown Source)
java.lang.NullPointerException: java.lang.NullPointerException


Personally, I'm in favor of the 1st option, ripping out _tostr() and just letting the runtime do its thing, but if people are _really_ passionate about preventing the user from tripping an NPE - despite their explicit wishes - then at least make it an optional debugging feature and not enforced baseline.

I still think that's wrong. It's too Java-ish and not truly Pythonic <g>, and the extent of inconsistency is going to be a problem for a long time to come (unless a lot of effort's put in to  large parts of the library, which doesn't seem likely or, frankly, justifiable).

What's your thoughts on the underlying-philosophical question?

I can make a patch for any of the above, but the underlying design philosophy is more significant and broader impact. How do you think Jython should proceed?


>I think it's too late in the release process for 2.2 to make the change
I suspected. I figured it'd be fine if there was a b3, but if release is imminent...sigh.
Ah well.

>Please continue working on it and submit a patch though.
>I'll happily put it in the 2.3 branch.
Then I'll happily proceed :->

1) I really like Jython doing type checking in os.path. The resulting error message is much more clearer than what you get in CPython. Are there some cases where a valid input would not be accepted? If not, I wouldn't change the behavior at all. If yes, I'd first try to make the type checking work correctly in those cases too.


2) In any case Java based exception should not come through. Otherwise code written for CPython may not work correctly. For example following try/except catches only Python exceptions but NPE would go through and possibly crash the whole program.

try:
    os.path.abspath(path)
except Exception, err:
    print "Got exception:", str(err)


3) This discussion should really happen in jython-devel.