Closed petrelharp closed 1 month ago
I'll tag @rdinnager here for his opinion, since he's been responsible for Windows portability issues.
The link you supplied seems, from my reading, to indicate that using /
is fine on all recent versions of DOS and Windows; the inconsistencies and issues with it sound ancient (and the post is, after all, on "retrocomputing"). Other apps on Windows might not handle /
properly, but that's of no concern to Eidos/SLiM, since the paths it uses internally will not be seen by other apps. DOS and Windows will do the right thing with /
, and that's what matters for our purposes.
But it does look to me like there is a bug with respect to looking for a separator by looking only for a /
. On Windows, we need to look for a separator by looking for either a /
or a \
, it seems to me. A cursory look at the code seems to indicate that this is a problem in several spots, not just Eidos_AbsolutePath()
; the same issue appears in Eidos_StripTrailingSlash()
, and Eidos_LastPathComponent()
, and so forth (I didn't attempt to find all cases of the problem).
So it looks like there's a need for some fixing and testing on Windows. I'm not the person to do that, since I don't have a Windows machine, don't know how to use developer tools on Windows, etc. So we need a volunteer who is on Windows – maybe @rdinnager, maybe @petrelharp since apparently you're using Windows for something?, maybe someone else? I'll mark this issue "help wanted" and await a PR, I guess. I'd advise searching for '/'
and "/"
in the code base, and checking all uses of it to make sure that there's a #ifdef _WIN32
branch that does the appropriate thing. What the appropriate thing is in all cases, I do not know. :->
In the meantime, it looks to me like the workaround is simply to use /
in all paths on Windows; the Eidos/SLiM code should handle those correctly (I think/hope), and DOS/Windows should as well (I think/hope). Just say no to \
.
Part of this work ought to be adding new unit tests, with #ifdef _WIN32
so they run only on Windows, that test all of these different filesystem functions with both /
and \
. Once somebody volunteers to do this work, I'll be happy to chat about that.
@samchamper how about you, you're on Windows right? Want to sit down together some time and work through this?
The link you supplied seems, from my reading, to indicate that using / is fine on all recent versions of DOS and Windows;
By my reading, mixed /
and \
worked fine in whatever particular tool they were using, but support was expected to be spotty and inconsistent. I agree that the 'looking for a /
' is the thing we know is a bug, though.
I'm only "using" windows by trying to run stdpopsim's CI on Windows (via github).
The link you supplied seems, from my reading, to indicate that using / is fine on all recent versions of DOS and Windows;
By my reading, mixed
/
and\
worked fine in whatever particular tool they were using, but support was expected to be spotty and inconsistent. I agree that the 'looking for a/
' is the thing we know is a bug, though.
The accepted answer there says (direct quote):
So using /
should be fine as long as we're talking to DOS/Windows, not to some random third-party software (which SLiM does not do directly). So I think we can take this bug as referring solely to the looking-only-for-/
thing. In any case, so far nobody has volunteered to help with this. Maybe I can figure out how to work on a fix using the GitHub Actions CI for Windows to test whether my fix is correct. I wish we had a go-to person for Windows issues.
OK, I have added tests and tweaked the Windows code, and my tests pass CI. I think that means this bug is fixed, but I can't test locally so who knows. I'm going to close this now, but if it is easy for you to test the original problem and confirm that it is fixed, @petrelharp, that would be helpful. Thanks!
Over in
Eidos_AbsolutePath( )
, if it figures out it's got an relative path, it (a) checks whether the end of thecurrent_dir
is a/
and if not, appends a/
; and then prependscurrent_dir
to the relative path, separated by a/
.Both of those
/
could cause problems on Windows - apparently Windows supports both/
and\
(!!??!) but inconsistently, but at least the first one could cause problems.