webkitpy.common.system.path_unittest fails with Cygwin 1.5

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Roben (:aroben)]

webkitpy.common.system.path_unittest fails with Cygwin 1.5

[Adam Roben (:aroben)]

Created attachment 72817 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 72817 [details]
Patch

What about non-absolute paths?  Or very short paths?

[Adam Roben (:aroben)]

(In reply to comment #2)
> (From update of attachment 72817 [details])
> What about non-absolute paths?  Or very short paths?

We always pass "-wa" to cygpath which instructs it to return an absolute Windows-style path. An absolute Windows path must be at least "C:\".

[Eric Seidel (no email)]

Comment on attachment 72817 [details]
Patch

OK.  Sad there isn't a nice way to test this change.  But looks OK.

[Dirk Pranke]

The intent of that function was to only deal with absolute paths; you could sort of tell from who calls it elsewhere in the module, but it's not at all clear. Adam, can you update the docstring for cygpath() to that effect? And maybe add a check that it's getting an absolute path and error out if it isn't?

Eric, what do you mean by "very short paths"? How would that be interesting?

[Adam Roben (:aroben)]

(In reply to comment #5)
> The intent of that function was to only deal with absolute paths; you could sort of tell from who calls it elsewhere in the module, but it's not at all clear. Adam, can you update the docstring for cygpath() to that effect? And maybe add a check that it's getting an absolute path and error out if it isn't?

It's fine to pass relative paths to cygpath(). They will be resolved against the current working directory and turned into an absolute path.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > The intent of that function was to only deal with absolute paths; you could sort of tell from who calls it elsewhere in the module, but it's not at all clear. Adam, can you update the docstring for cygpath() to that effect? And maybe add a check that it's getting an absolute path and error out if it isn't?
> 
> It's fine to pass relative paths to cygpath(). They will be resolved against the current working directory and turned into an absolute path.

Yes, I know. However, I don't want callers using the function that way.

[Dirk Pranke]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > > The intent of that function was to only deal with absolute paths; you could sort of tell from who calls it elsewhere in the module, but it's not at all clear. Adam, can you update the docstring for cygpath() to that effect? And maybe add a check that it's getting an absolute path and error out if it isn't?
> > 
> > It's fine to pass relative paths to cygpath(). They will be resolved against the current working directory and turned into an absolute path.
> 
> Yes, I know. However, I don't want callers using the function that way.

At least, not until there's a clear need for it.

[Dirk Pranke]

To restate my last comment another way ... I'm trying to be very careful about how we use actual filesystem paths in our code. In NRWT especially, I'm trying to move all of the code to use "test names" instead of files, where the test name is a unix-style path relative some port-specific root(s) (normally layout_test_dir). There are a lot of confusion and bugs that arise from not knowing when something is a test name, a native relative path, a native absolute path, a unix-style absolute path, or a URI (this is where the weak typing of Python is hurting us).

So, I'd like to be clear about what should be being passed in to cygpath(), so that we can catch potentially incorrect usages of it that might reflect someone thinking they had (or might have) a path of type X when it should be (or will only be) type Y.

Now, one could argue that if cygpath is intended to be a generic routine used by other places in webkitpy, this assertion should be enforced in the port code above the call to cygpath.

However, I think we have a general portability issue here in test code that will probably require similar sorts of restrictions in order to be clear and consistent in all of our code and tests, so I'd like to try to start with a conservative approach and only relax it if necessary. Since that's much easier than the other direction :)

[Adam Roben (:aroben)]

(In reply to comment #9)
> In NRWT especially, I'm trying to move all of the code to use "test names" instead of files, where the test name is a unix-style path relative some port-specific root(s) (normally layout_test_dir). There are a lot of confusion and bugs that arise from not knowing when something is a test name, a native relative path, a native absolute path, a unix-style absolute path, or a URI (this is where the weak typing of Python is hurting us).

Yes, I've noticed this, too, while working on bug 48517. I actually have some patches that move us in that direction (enough to get the tests passing in Win32 Python) which I'll be posting soon.

> So, I'd like to be clear about what should be being passed in to cygpath(), so that we can catch potentially incorrect usages of it that might reflect someone thinking they had (or might have) a path of type X when it should be (or will only be) type Y.

OK, understood. Thanks!

[Adam Roben (:aroben)]

Committed r71321: <http://trac.webkit.org/changeset/71321>