inducer / relate

RELATE is an Environment for Learning And TEaching
http://documen.tician.de/relate
Other
383 stars 118 forks source link

Windows path handling is broken #767

Open inducer opened 3 years ago

inducer commented 3 years ago

As discussed here.

cc @dzhuang

inducer commented 3 years ago

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

inducer commented 3 years ago

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).

dzhuang commented 3 years ago

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.

inducer commented 3 years ago

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.)

dzhuang commented 3 years ago

Got it. I'll have a try.

dzhuang commented 3 years ago

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?

inducer commented 3 years ago
{% 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.

dzhuang commented 3 years ago

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.

inducer commented 3 years ago

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.