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

Cross-platform addons: file names case-sensitivity #2046

Open
vn971 opened this Issue Sep 26, 2017 · 21 comments

Comments

Projects
None yet
7 participants
@vn971
Contributor

vn971 commented Sep 26, 2017

The goal of this proposal is to make resource loading in wesnoth cross-platform, i.e. working identically on case-sensitive and case-insensitive filesystems. This tries to eliminate the current problem of some addons working on one OS and not working on the other. A particularly popular problem is addons tested on Windows (which is case-insensitive by default), yet not working on other OSes.

One proposal is to make resource loading case-sensitive (on all platforms). This will show mistakes immediately if the addon developer uses a wrong case.

Another proposal (made in comments below) is to make resource loading case-insensitive on all platforms, meaning that wesnoth will load resources that have a different case. (Even on case-sensitive filesystems.)

Thoughts?

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Sep 29, 2017

Contributor

This tries to eliminate the current problem of Windows addon developers testing their addon on Windows and having no idea it won't work on other OSes.

Don't we also have this problem the other way around? If a linux umc developer puts 2 files in an addon that only differ by case but have othwewise the same name, it won't work on windows.

Contributor

gfgtdf commented Sep 29, 2017

This tries to eliminate the current problem of Windows addon developers testing their addon on Windows and having no idea it won't work on other OSes.

Don't we also have this problem the other way around? If a linux umc developer puts 2 files in an addon that only differ by case but have othwewise the same name, it won't work on windows.

@vn971

This comment has been minimized.

Show comment
Hide comment
@vn971

vn971 Sep 29, 2017

Contributor

@gfgtdf good point!

Contributor

vn971 commented Sep 29, 2017

@gfgtdf good point!

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Sep 29, 2017

Contributor

Consider a folder with 10000 files. One of those files is mYfile. How do i match it to MYFILE? I can easily convert MYFILE to myfilebut I need to enumerate the entire 10000 files to find and convert mYfile tomyfile. And then I am left with what to do if mYfILe also appears.

Contributor

GregoryLundberg commented Sep 29, 2017

Consider a folder with 10000 files. One of those files is mYfile. How do i match it to MYFILE? I can easily convert MYFILE to myfilebut I need to enumerate the entire 10000 files to find and convert mYfile tomyfile. And then I am left with what to do if mYfILe also appears.

@vn971

This comment has been minimized.

Show comment
Hide comment
@vn971

vn971 Sep 29, 2017

Contributor

@GregoryLundberg the proposal for the server is to refuse all addons that have at least 2 files with the same case-ignore name.
Technically, that's trivial to do: map all names to their lowercase values and find if there are duplicates. Finding these duplicates is easily and effectively done with a HashSet.

Contributor

vn971 commented Sep 29, 2017

@GregoryLundberg the proposal for the server is to refuse all addons that have at least 2 files with the same case-ignore name.
Technically, that's trivial to do: map all names to their lowercase values and find if there are duplicates. Finding these duplicates is easily and effectively done with a HashSet.

@GregoryLundberg

This comment has been minimized.

Show comment
Hide comment
@GregoryLundberg

GregoryLundberg Sep 30, 2017

Contributor

What I am trying to point out is that, while the proposal is all well-and-good, it presumes a perfect gate-keeper and can fail is amazing and surprising ways if something happens inside the walled garden.

Contributor

GregoryLundberg commented Sep 30, 2017

What I am trying to point out is that, while the proposal is all well-and-good, it presumes a perfect gate-keeper and can fail is amazing and surprising ways if something happens inside the walled garden.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Sep 30, 2017

Contributor

The opposite issue is less severe, since it's harder to do by mistake.

EDIT: though it seems, installing such addon will only create one file out of several silently.

A trailing dot in filename causes same effect as different capitalization.

Contributor

Arcanister commented Sep 30, 2017

The opposite issue is less severe, since it's harder to do by mistake.

EDIT: though it seems, installing such addon will only create one file out of several silently.

A trailing dot in filename causes same effect as different capitalization.

@vn971 vn971 changed the title from Make resource names case-sensitive to Cross-platform addons: make resource names case-sensitive Sep 30, 2017

@gfgtdf

This comment has been minimized.

Show comment
Hide comment
@gfgtdf

gfgtdf Sep 30, 2017

Contributor

i have already seen projects that ship with files with the same (case insensitive) name, even though it was just the INSTALL file it was quit annyoing sinc my first unpack programm simply refused to unpack open it. I have to say though that i don't know exactly how windows filesystem behaves when it comes to non-ascii chracters. If we were currently initially creatign the adodn server i'd probably suggest to just only allow lowercase filenames but currently this might break too many addons.

Contributor

gfgtdf commented Sep 30, 2017

i have already seen projects that ship with files with the same (case insensitive) name, even though it was just the INSTALL file it was quit annyoing sinc my first unpack programm simply refused to unpack open it. I have to say though that i don't know exactly how windows filesystem behaves when it comes to non-ascii chracters. If we were currently initially creatign the adodn server i'd probably suggest to just only allow lowercase filenames but currently this might break too many addons.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Sep 30, 2017

Contributor

Installing such add-on on Windows doesn't spit out any errors. Files get overwritten silently.

Contributor

Arcanister commented Sep 30, 2017

Installing such add-on on Windows doesn't spit out any errors. Files get overwritten silently.

@vn971 vn971 changed the title from Cross-platform addons: make resource names case-sensitive to Cross-platform addons: case-sensitive file names Sep 30, 2017

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 1, 2017

Member

Um, the title of this issue seems backwards to me. It should treat filenames as case-insensitive, not case-sensitive. (And if @gfgtdf is correct, also reject filenames ending in a period. More specifically, it should reject any filenames that would be illegal on Windows.)

And rather than coercing to lowercase, I'd suggest using a case-insensitive collator which considers 'a' and 'A' to be equal. (I don't quite remember how to do this, but something similar to translation::icompare should work.)

Member

CelticMinstrel commented Oct 1, 2017

Um, the title of this issue seems backwards to me. It should treat filenames as case-insensitive, not case-sensitive. (And if @gfgtdf is correct, also reject filenames ending in a period. More specifically, it should reject any filenames that would be illegal on Windows.)

And rather than coercing to lowercase, I'd suggest using a case-insensitive collator which considers 'a' and 'A' to be equal. (I don't quite remember how to do this, but something similar to translation::icompare should work.)

@vn971

This comment has been minimized.

Show comment
Hide comment
@vn971

vn971 Oct 1, 2017

Contributor

@CelticMinstrel well, it was meant from the following perspective: when you load any resource (or any addon file at all), you do it in a case-sensitive manner, even on Windows. So if you requested file "aBC.cfg", wesnoth won't load "abc.CFG" even on Windows.

The alternative you write about (making wesnoth working case-insensitively on all platforms) could work, too.

Contributor

vn971 commented Oct 1, 2017

@CelticMinstrel well, it was meant from the following perspective: when you load any resource (or any addon file at all), you do it in a case-sensitive manner, even on Windows. So if you requested file "aBC.cfg", wesnoth won't load "abc.CFG" even on Windows.

The alternative you write about (making wesnoth working case-insensitively on all platforms) could work, too.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Oct 1, 2017

Contributor

I think, case sensitive is overall better. It should also be easier to implement:
When running on OS like Windows or MacOS X, first locate needed resource file, then if its case doesn't match, throw an error or at least clearly-visible warning like:

"units/Naga_Bowmaster.cfg" requested "Attacks/Bow.png", but found "images/attacks/bow.png". Either rename the resource file or correct WML code.

Contributor

Arcanister commented Oct 1, 2017

I think, case sensitive is overall better. It should also be easier to implement:
When running on OS like Windows or MacOS X, first locate needed resource file, then if its case doesn't match, throw an error or at least clearly-visible warning like:

"units/Naga_Bowmaster.cfg" requested "Attacks/Bow.png", but found "images/attacks/bow.png". Either rename the resource file or correct WML code.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 1, 2017

Member

I guess there are two sides to this.

  1. The WML preprocessor and resource resolver should give an error if it requests a file but then finds that the case does not match. Like @Arcanister said, if an addon requests "Attacks/Bow.png", but the canonical path is actually "attacks/bow.png", that should be an error. Similarly, if you include {./units.cfg} but the file is actually called "units.CFG", that should be an error.
  2. The add-on uploader should reject any add-on containing two files that differ only by case. For example, if the add-on contains "attacks/bow.png" and also "attacks/bow.PNG", it should not allow you to upload it.

For point 1 it would be possible to instead search for files case-insensitively even on a case-sensitive filesystem, but I think that would be more difficult and error-prone.

Member

CelticMinstrel commented Oct 1, 2017

I guess there are two sides to this.

  1. The WML preprocessor and resource resolver should give an error if it requests a file but then finds that the case does not match. Like @Arcanister said, if an addon requests "Attacks/Bow.png", but the canonical path is actually "attacks/bow.png", that should be an error. Similarly, if you include {./units.cfg} but the file is actually called "units.CFG", that should be an error.
  2. The add-on uploader should reject any add-on containing two files that differ only by case. For example, if the add-on contains "attacks/bow.png" and also "attacks/bow.PNG", it should not allow you to upload it.

For point 1 it would be possible to instead search for files case-insensitively even on a case-sensitive filesystem, but I think that would be more difficult and error-prone.

@Ja-MiT

This comment has been minimized.

Show comment
Hide comment
@Ja-MiT

Ja-MiT Oct 1, 2017

Contributor

@vn971 when you state a preference for case-sensitive or case-insensitive, you are going beyond stating the problem into the realm of proposing one solution over the other. If you want to give your issue a better shot at the best solution (however "best" should be determined), I would suggest a solution-neutral title for the issue, such as "case-sensitivity of file names". I would also suggest that the goal of this issue be "to eliminate the current problem of Windows addon developers testing their addon on Windows and having no idea it won't work on other OSes" rather than insisting on the specific proposal "to make resource loading in wesnoth case-sensitive (on all platforms)".

(This is intended as advice. I've seen things work out poorly sometimes when people start out focused on a particular solution rather than focused on the problem.)

Contributor

Ja-MiT commented Oct 1, 2017

@vn971 when you state a preference for case-sensitive or case-insensitive, you are going beyond stating the problem into the realm of proposing one solution over the other. If you want to give your issue a better shot at the best solution (however "best" should be determined), I would suggest a solution-neutral title for the issue, such as "case-sensitivity of file names". I would also suggest that the goal of this issue be "to eliminate the current problem of Windows addon developers testing their addon on Windows and having no idea it won't work on other OSes" rather than insisting on the specific proposal "to make resource loading in wesnoth case-sensitive (on all platforms)".

(This is intended as advice. I've seen things work out poorly sometimes when people start out focused on a particular solution rather than focused on the problem.)

@vn971

This comment has been minimized.

Show comment
Hide comment
@vn971

vn971 Oct 1, 2017

Contributor

@Ja-MiT of course I don't push one solution or the other. This is why I wrote earlier "could work, too". The proposal I posted is only a proposal (the one that came to my mind), I don't insist. After all, this is what comments are made for. I'll try to rename the issue better.

EDIT: I've updated both the issue title and the first comment.

Contributor

vn971 commented Oct 1, 2017

@Ja-MiT of course I don't push one solution or the other. This is why I wrote earlier "could work, too". The proposal I posted is only a proposal (the one that came to my mind), I don't insist. After all, this is what comments are made for. I'll try to rename the issue better.

EDIT: I've updated both the issue title and the first comment.

@vn971 vn971 changed the title from Cross-platform addons: case-sensitive file names to Cross-platform addons: file names case-sensitivity Oct 1, 2017

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 5, 2017

Member

I'm happy to write case checks for windows. I can't test them locally, but I can push some code for other people to test.

The problem is what to do when a case mismatch is detected. I can write to the log, but windows people rarely read those.

As I don't want to add GUI calls into the filesystem code, that basically just leaves the option of refusing to read the file, and hoping the developer notices.

Member

AI0867 commented Oct 5, 2017

I'm happy to write case checks for windows. I can't test them locally, but I can push some code for other people to test.

The problem is what to do when a case mismatch is detected. I can write to the log, but windows people rarely read those.

As I don't want to add GUI calls into the filesystem code, that basically just leaves the option of refusing to read the file, and hoping the developer notices.

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 5, 2017

Member

If anyone would like to test it, here's a branch implementing case sensitivity on windows: https://github.com/AI0867/wesnoth/tree/windows-case-check

This check is based on the WIN32 API, so it won't do the same for macOS, which by default, uses HFS+ in case-insensitive mode.

Member

AI0867 commented Oct 5, 2017

If anyone would like to test it, here's a branch implementing case sensitivity on windows: https://github.com/AI0867/wesnoth/tree/windows-case-check

This check is based on the WIN32 API, so it won't do the same for macOS, which by default, uses HFS+ in case-insensitive mode.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 6, 2017

Member

That check looks good, but obviously we'd need a MacOS version of it too before pushing it to master. I'm not sure how that would be done.

Member

CelticMinstrel commented Oct 6, 2017

That check looks good, but obviously we'd need a MacOS version of it too before pushing it to master. I'm not sure how that would be done.

@Arcanister

This comment has been minimized.

Show comment
Hide comment
@Arcanister

Arcanister Oct 6, 2017

Contributor

In addition to that, resource loading and expanding file inclusion should threat \ as escaping character instead of path separator, even on Windows

Contributor

Arcanister commented Oct 6, 2017

In addition to that, resource loading and expanding file inclusion should threat \ as escaping character instead of path separator, even on Windows

@vn971

This comment has been minimized.

Show comment
Hide comment
@vn971

vn971 Oct 6, 2017

Contributor

@AI0867 I can't test personally because I don't have Windows.
IIRC windows GUI programs do not even have console logs (there are strange rules to declare yourself either as GUI app or as console app, something like that).

Contributor

vn971 commented Oct 6, 2017

@AI0867 I can't test personally because I don't have Windows.
IIRC windows GUI programs do not even have console logs (there are strange rules to declare yourself either as GUI app or as console app, something like that).

@AI0867

This comment has been minimized.

Show comment
Hide comment
@AI0867

AI0867 Oct 6, 2017

Member

@vn971 yes, we log to stdout.log and stderr.log on windows, if possible.

Member

AI0867 commented Oct 6, 2017

@vn971 yes, we log to stdout.log and stderr.log on windows, if possible.

@CelticMinstrel

This comment has been minimized.

Show comment
Hide comment
@CelticMinstrel

CelticMinstrel Oct 7, 2017

Member

I don't think \ should be treated as an escape character. It should be treated as a path separator. If you absolutely must have an escape character, I'd recommend using % followed by two hex digits, since that's an already-established format in paths (URLs to be more precise, but they're closely related). Still, I don't think it's necessary.

@vn971 Windows programs can be both GUI and console, which results in a console window behind your program window. You can get this in Wesnoth by passing --wconsole.

Member

CelticMinstrel commented Oct 7, 2017

I don't think \ should be treated as an escape character. It should be treated as a path separator. If you absolutely must have an escape character, I'd recommend using % followed by two hex digits, since that's an already-established format in paths (URLs to be more precise, but they're closely related). Still, I don't think it's necessary.

@vn971 Windows programs can be both GUI and console, which results in a console window behind your program window. You can get this in Wesnoth by passing --wconsole.

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