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

Handling relative paths. Confusion with deconstructPathname #264

Closed
chrisdembia opened this issue Nov 11, 2014 · 13 comments
Closed

Handling relative paths. Confusion with deconstructPathname #264

chrisdembia opened this issue Nov 11, 2014 · 13 comments

Comments

@chrisdembia
Copy link
Member

I am working on opensim-org/opensim-core#118. @sherm1 , in this issue you claim that

FYI, Simbody has a utility class SimTK::Pathname that provides platform-independent handling of relative path names so that changing directory isn't required.

Based on the Pathname API, this does not seem possible. So I have been trying to alter Pathname::getAbsolutePathname to suit my needs.

My objective is to take a path pathname that is relative an arbitrary path base, and express it as either (1) a relative path that is relative to my cwd, or (2) an absolute path.

I made the necessary changes to getAbsolutePathname so that it could use a base that is not the cwd. However, I misunderstood what deconstructPathname does. The documentation contains the confusing statement:

"." starts an absolute path name which is relative to the current working directory

How can an absolute path be relative to something else? I fed this method a path like ../../myfile and was told that this is an absolute path. I learned that deconstructPathname is trying to do more than it advertises: it converts relative paths to absolute paths if you pass it "keywords" such as ../, ./, @. So from this perspective, indeed, ./ specifies an absolute path. I feel this is improper; getAbsolutePathname should do that; I simply wanted to deconstruct the path as given.

I believe my current method of trying to do (1) or (2) cannot be done simply by modifying getAbsolutePathname.

@sherm1 does Pathname, as is, provide the functionality I seek? If not, I would like to alter deconstructPathname so that it strictly decomposes a given path, and does not try to create an absolute path based on keywords. I'm happy to take other suggestions as well.

@chrisdembia
Copy link
Member Author

K maybe scratch some of what I said above. Now I think what I want is for ../ not to be prepended with ./ within deconstructPathname, since just because I want to go up a directory doesn't mean that I want to go up a dir. from my cwd.

@sherm1
Copy link
Member

sherm1 commented Nov 12, 2014

Hi, Chris. This stuff is very difficult to think about clearly. As best I can tell deconstructPathname() does exactly what its documentation says; what's not immediately obvious is why it ought to behave that way. However, its main goal in life is to distinguish between particular file specifications and files that might be subject to a search path. A file like geometry/mesh.obj can be subject to a search path, while ./geometry/mesh.obj is actually a specification of a particular file relative to the current working directory. This rule is followed, for example, by shells when you type a command; ls uses the search path, while ./ls would look in the current directory. These distinctions are made extra difficult by having to deal with Windows and Unix together. In particular the drive letter on Windows is a nightmare. Consider: geometry/mesh.obj is relative, but c:geometry/mesh.obj is defined (by Windows) to be relative to drive C's current working directory (Windows keeps a per-drive cwd).

That is the kind of crap that deconstructPathname() is built to sort out.

I am not sure I understand exactly what you are trying to do, but I wonder if a simple extension to deconstructPathname() might do what you want. Currently a file beginning with "." is processed by replacing the "." with the absolute cwd. It sounds like maybe you would like to use that same syntax, but where "." refers to a particular directory of your choosing, rather than the cwd, and ".." would mean one up from your directory, while processing of root-relative path names would remain unchanged.

That could easily be done by adding a new Pathname method deconstructPathnameAgainstGivenDirectory() that would be just like deconstructPathname() but with the cwd passed in. Then deconstructPathname() would be implemented as a call to the new method but passing in the actual cwd. We would need to figure out the correct behavior for the case where the given directory is not an absolute path -- probably it should be passed through getAbsolutePathname() before being used.

@chrisdembia
Copy link
Member Author

I am not sure I understand exactly what you are trying to do, but I wonder if a simple extension to deconstructPathname() might do what you want.

That would work, mostly, but it still seems to me that a path like ../../hello is a valid relative path, and I'm currently modifying deconstructPathname to be able to handle that type of path as a relative path. Maybe I don't know what I'm getting into though... you say here that starting with ".." is ill-formed. Why is that?

@sherm1
Copy link
Member

sherm1 commented Nov 12, 2014

but it still seems to me that a path like ../../hello is a valid relative path

By the definition used in deconstructPathname(), that is an absolute path because it is cwd relative (and cwd is an absolute path). You should definitely not change that behavior, because then you can't use deconstructPathname() to determine whether you need to apply a search path. I should have used some different terms than relative/absolute here; what's meant is "search path should apply"/"search path should not apply".

I still think that deconstructPathname() does what you want, except that you want to supply your own effective "cwd". But from what you've said I don't see that any of the other behavior should change at all. Have you thought through what should happen if the external force file contains a path name like "c:../blah.mot" and whether that should be treated differently than "../blah.mot"?

@sherm1
Copy link
Member

sherm1 commented Nov 12, 2014

you say here that starting with ".." is ill-formed. Why is that?

I believe that is a poorly-worded attempt to say that while processing a path like ../../../../blah it is ill-formed if you run off the top of root.

In the code, a given path name that actually does start with .. is modified to start with ./.. so that it doesn't require special casing. So that is definitely not ill formed.

@sherm1
Copy link
Member

sherm1 commented Dec 17, 2014

@carmichaelong, I thought of one more ugly Windows case I don't think we discussed:
Say my current drive is "C:". Suppose:

specifiedWorkingDirectory="/users/sherm/documents"
pathName="X:myfile.txt"

What should be the absolute pathname in that case? My thought is that since only one drive was explicitly mentioned, that drive should be used. So the absolute path name would be:

X:/users/sherm/documents/myfile.txt

But in that case it would be wrong to pre-process the specifiedWorkingDirectory (swd) to turn it into an absolute path first! So maybe the rule is:

  • if the swd has a drive letter, then that's the drive to use for a working-directory relative path name, possibly ignoring the path's drive if it conflicts
  • otherwise if the path has a drive letter, apply that to the swd also
  • otherwise use the current drive for the swd

@chrisdembia
Copy link
Member Author

Why does Windows exist!

@sherm1
Copy link
Member

sherm1 commented Dec 18, 2014

To summarize a conversation Chris, Carmichael, and I just had:

  • deconstructPathname() is mixing together several high level policies with the string processing (meaning of . and @ and searchable/nonsearchable distinction)
  • what is needed for OpenSim is a routine like getAbsolutePathname(swd, path)
  • Carmichael will rip out the guts of deconstructPathname() to make a lower-level method that eliminates the policies and just processes a given path against the swd, canonicalizing it and dealing with the drive specs on Windows.
  • Then Carmichael will implement the new getAbsolutePathname() method using the new guts
  • Then we'll see if we can re-implement the old deconstructPathname() and old getAbsolutePathname() methods in terms of the new guts also

Thanks, guys!

@chrisdembia
Copy link
Member Author

Thanks for summarizing.

@carmichaelong
Copy link
Contributor

The above case of ambiguous drive letters makes sense. What about the following case?

swd = "Y:/folder"
pathName="/users/sherm/documents/myfile.txt"

Assume that the current drive is X:. Do we return:

  1. X:/users/sherm/documents/myfile.txt
  2. Y:/folder/users/sherm/documents/myfile.txt

Case 1 would imply that pathName is a full path (this is unambiguous in not-Windows). Case 2 implies that having a drive letter dominates in Windows.

@sherm1
Copy link
Member

sherm1 commented Dec 18, 2014

What about the following case? (swd=Y:/folder, path=/users/sherm/documents/myfile.txt)

Yuck! Good catch, Carmichael. One way to think about that is how we would want it to behave in the OpenSim case we've been discussing. That would mean we are processing a file like Y:/folder/external_force.xml, and inside that xml file we found a reference to /users/sherm/documents/myfile.txt. What should that mean? I guess it probably means the author was working on Linux and someone is now trying to use it on Windows where it isn't going to work. In that case maybe the best way to determine the behavior is to return whatever will produce the nicest error message when an attempt is made to open the non-existent file.

My first thought is that we should interpret the path as an absolute path name but on the swd drive. So I would propose (weakly) that the answer is a case 3:
3) Y:/users/sherm/documents/myfile.txt

@sherm1
Copy link
Member

sherm1 commented May 15, 2015

Chris, I believe PR #307 has addressed this issue which you opened. Please close if you agree.

@chrisdembia
Copy link
Member Author

🎆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants