Glk: glk_stream_open_file and non existent files
curiousdannii opened this issue · 11 comments
(Follow up to these comments)
The spec for glk_stream_open_file currently says:
If the filemode requires the file to exist, but the file does not exist, glk_stream_open_file() returns NULL.
But many Glk libraries will actually issue a fatal error. The spec should be edited to say that the file must exist instead. Possibly there could be a comment saying that interpreters can keep their current lax behaviour, but that game authors must ensure the file exists. (Edit 29 Oct 2017, I don't think this would really be all that helpful - lax IDE implementations is a big cause of problems.)
Now the Inform 7 template function RESTORE_THE_GAME_R doesn't check for existence before trying to open a stream. The spec says this for glk_fileref_create_by_prompt:
filemode_Read: The file must already exist; the player will be asked to select from existing files which match the usage.
I had interpreted this as more of a hint to the library/player - particularly with GUIs that the library is to present the player with a list of files. But perhaps it was intended to be more determinative.
The spec for glk_fileref_create_by_prompt could be clarified to say that libraries must validate that the file entered by the player does actually exist if the mode is filemode_Read, or else the library must return NULL, even though the player did select something. I think this would keep the code pattern followed by Inform etc working.
I ran into this again today, cause the Windows IDE is lax but Quixe isn't. Could we have a decision either way soon, so I can start asking terps to become compliant? :)
So, now that I've looked at some code...
You're right that the interpreters don't match the spec here. Glkote has the same behavior: on filemode_Read, if the file does not exist, it throws an exception.
The spec has always said that glk_stream_open_file returns null in this case, but the Unix interpreters have always generated a warning (though not a fatal error). This is a nuisance. Clearly the sensible behavior for games is to always check before calling open_file. But then, as you say, RESTORE_THE_GAME_R relies on the file prompt selecting an existing file.
I feel like it's worth changing my libraries (cheapglk/glkterm/remglk/glkote) to match the spec. I would then add a comment to the spec saying that in practice this is going to cause an error, so don't do it. Maybe that's being over-respectful to the spec, though -- I can see the argument for just documenting what real code does.
The behavior for glk_fileref_create_by_prompt varies a bit. In cheapglk, you can enter the name of a nonexistent file (there's no check), but then open_file just prints a warning. So that works out. In glkote you can only select an existing file. But it's possible that in Win/Linux Lectrote, you can type the name of a nonexistent file in the selection dialog. I haven't tested that case.
The spec has always said that glk_stream_open_file returns null in this case, but the Unix interpreters have always generated a warning (though not a fatal error). This is a nuisance. Clearly the sensible behavior for games is to always check before calling open_file. But then, as you say, RESTORE_THE_GAME_R relies on the file prompt selecting an existing file.
I feel like it's worth changing my libraries (cheapglk/glkterm/remglk/glkote) to match the spec. I would then add a comment to the spec saying that in practice this is going to cause an error, so don't do it. Maybe that's being over-respectful to the spec, though -- I can see the argument for just documenting what real code does.
Because of the speed at which IF terps get updated, I think it would be best to change the spec to be stricter. You're always safe to test with glk_fileref_does_file_exist even on the oldest terps, but if the current spec is kept and the strict terps are changed, then people will still get caught out when playing on old or uncommon platforms.
The behavior for glk_fileref_create_by_prompt varies a bit. In cheapglk, you can enter the name of a nonexistent file (there's no check), but then open_file just prints a warning. So that works out. In glkote you can only select an existing file. But it's possible that in Win/Linux Lectrote, you can type the name of a nonexistent file in the selection dialog. I haven't tested that case.
In Gargoyle and Lectrote on Windows the file dialog shows an error if you try typing a non-existent file. I can't remember anyone ever reporting fatal errors when opening non-existent files from their standard Inform projects before. So possibly this problem is not one that has appeared in practice, and I've only noticed it because of my crappy glkote-term implementation.
Changing the spec to say glk_fileref_create_by_prompt must validate the file exists is still an option, and a good one IMO, but perhaps all that is needed is a couple of non-binding notes: one to implementation authors recommending validation (if their OS doesn't already do so), and one to game authors saying that not all Glk implementations do validate, so using glk_fileref_does_file_exist is recommended.
If the spec for glk_stream_open_file is changed but glk_fileref_create_by_prompt isn't, then I can submit issues/pull requests to I6/I7 to fix RESTORE_THE_GAME_R for future works.
@DavidKinder Any thoughts?
Sorry for the delay in replying - I don't often log onto Github and have only just noticed that there was a notification that you've tagged me. My initial thought on all this is that I like how the specification currently reads for glk_stream_open_file(). While you can check that the file exists before calling, you've still got no absolute guarantee that the file won't disappear between checking and calling - some other process could remove the file or make it inaccessible.
If it were my choice I'd leave glk_stream_open_file() as it is in the specification, and perhaps change glk_fileref_create_by_prompt() to say that libraries should do as much as possible to ensure that the file already exists in the read-only case, then hassle Glk implementers to match that.
Gah, decision time or I'll forget this issue exists entirely.
First note: glk_fileref_create_by_prompt() already says
filemode_Read: The file must already exist; the player will be asked to select from existing files which match the usage.
So the spec says to do it and cheapglk is not holding up its end.
Second note:
While you can check that the file exists before calling, you've still got no absolute guarantee that the file won't disappear between checking and calling
That is true, but it is so far down my list of worries that it's not on my list of worries. Sorry! It just isn't something that will ever be relevant to IF interpreters as person-plays-a-game tools.
(But it could become important if you're using Glulx in some unexpected context, like for a compiler or text-processing tool. Which could happen!)
Okay, on balance, I am going to keep the spec definition of glk_stream_open_file(). I will adjust my interpreters to return null on missing files for that case. I will add a spec note saying that legacy code may throw a fatal error if you try to open a nonexisting file for reading.
I will adjust cheapglk's glk_fileref_create_by_prompt() so that if you type a nonexistent filename for a Read prompt, it silently returns null.
I realize this leaves us with an effectively indefinite future in which legacy interpreters don't match the spec. Updating RESTORE_THE_GAME_R will only help games released after the next I7 release, which is its own species of indefinite. But the other option has its own long-term problems, so I am optimistically taking the view that all software is updated eventually.
(FWIW, I had lunch with Graham a couple of weeks ago and he said he was back into a phase of regular I7 work...)
Spec note:
Unfortunately, many (most) older interpreters will throw a fatal error in this case (missing file for filemode_Read) rather than returning NULL. Therefore it is best to call glk_fileref_does_file_exist() before trying to read a file.
Thank you for making the call!
So if we're going to be keeping the current glk_stream_open_file definition, should interpreters which current print a warning (even if you can continue playing) be changed to just silently return null?
Closing, see erkyrath/cheapglk#4 and erkyrath/cheapglk#3 .
If I see other terps which have fatal or non-fatal errors, I'll point them to this discussion.
Doesn't look like externalfile.inf tests this currently. Would be good to have a test of the specced behaviour.