relate icon indicating copy to clipboard operation
relate copied to clipboard

Windows path handling is broken

Open inducer opened this issue 5 years ago • 9 comments

As discussed here.

cc @dzhuang

inducer avatar Feb 03 '21 19:02 inducer

In particular, that means we shouldn't use os.path functions for path processing.

inducer avatar Feb 03 '21 19:02 inducer

The way I understand path handling in Relate on Windows right now is that one would use \\ as a path separator (e.g. in Jinja includes), whereas one would use / on Linux. This means that you can't use a Linux-based Relate repo on a Windows Relate and vice versa. Is my understanding accurate here? If so, IMO this is a problem that should be easy to avoid, perhaps best by standardizing on / as a path separator (and hence not using os.path to process paths, which uses system-dependent separators).

inducer avatar Feb 25 '21 06:02 inducer

Thanks for your explanation, I think your suggestion is reasonable because my patch might result in failure. However, In my case, all my repos can work on both Windows (for debugging) and Linux (for production). Can you provide the traceback which lead to this issue? I want to create a testcase which actually fix that, thanks.

dzhuang avatar Feb 27 '21 09:02 dzhuang

In your repos, what do you use for path separators, e.g. when using Jinja:

{% from "path1/path2/yaml-macros.jinja" import hw_header %}

? (You may not have any of those that have directory components in them. In that case, please test them, because I strongly suspect they're broken.)

inducer avatar Feb 28 '21 22:02 inducer

Got it. I'll have a try.

dzhuang avatar Mar 01 '21 09:03 dzhuang

In your repos, what do you use for path separators, e.g. when using Jinja:

I used / in my repos. FYI, I used \ only when configuring GIT_ROOT in local_settings, e.g., GIT_ROOT = r"D:\document\python\course\course-git" on Windows.

For this kind of snippet:

{% from "path1/path2/yaml-macros.jinja" import hw_header %}

I can do macro import from 2-level subfolder without issue both on Windows and Linux. Am I missing anything?

dzhuang avatar Mar 01 '21 09:03 dzhuang

{% from "path1/path2/yaml-macros.jinja" import hw_header %}

That's surprising to me, given that this code would try to split the path by \ and thus try to ask Dulwich for non-existent file names.

inducer avatar Mar 01 '21 21:03 inducer

I notice there're a few commits I didn't merge into my fork, which might cause the exceptions you mentioned. I'll test it again in a few days.

dzhuang avatar Mar 02 '21 00:03 dzhuang

Thanks! The changes are about support for symlinks in the repo. They should (I think) not affect the behavior I describe one way or another. But since I had to touch that code, I ended up noticing this (potential?) problem.

inducer avatar Mar 02 '21 00:03 inducer