Skip to content
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

BACKENDS: Rename and simplify AbstractFSNode::create() #1727

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ccawley2011
Copy link
Member

commented Jul 1, 2019

Nothing ever used create() to create files, and the same effect could be achieved using createReadStream(), so create() has been simplified to only create directories. In addition, createDirectory() will return true if the directory already exists, and the check for existence is done by the calling code.

The changes to Common::FSNode are originally from PR #1720.

The PSP and chroot changes have not been tested.

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 1, 2019

As mentioned in the other PR, this feature isn’t implemented in most of our backends, and the practical uses of it are quite specific. We do not allow directory creation or traversal for the vast majority of our supported games. For example, we disallow changing of the save directory in SCI games, as it would be a nightmare to have several different save game directories.

Thus, I’m personally against this feature, for portability and practical reasons.

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

Also refer to the discussion in PR #366

@criezy

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

See also https://lists.scummvm.org/pipermail/scummvm-devel/2013-August/011006.html

If creating directories is indeed an issue with some ports, I agree that it might be better to find an alternative solution rather than expose it in FSNode. The change to FSAbtractNode itself (and the rest of the backend code) seems good through.

But I am a bit confused. Some code seems to already create directories, such as the networking code, and more interestingly Common::DumpFile. And also why we have the ability to create directories in FSAbstractNode if we don't want it to be used? So to me it looks like it is half-allowed, and half not, and it is confusing. We might want to make a decision one way or the other.

@ccawley2011 ccawley2011 force-pushed the ccawley2011:createDirectory branch from 1069d0a to 61c72b0 Jul 2, 2019

@tsoliman

This comment has been minimized.

Copy link
Member

commented Jul 2, 2019

My 2 cents is that platform backends should be able to create directories if they want (e.g. create the directory to store the ScummVM ini file because it's that kind of platform) but engines should not.

@bluegr

This comment has been minimized.

Copy link
Member

commented Jul 5, 2019

I see that this PR adds untested code for the chroot and PSP backends. We still have about 6-7 backends that do not support directory creation. Also, with these changes, when a backend does not support directory creation, it’s just a warning instead of an error, so the fallback is to skip directory creation altogether.

The best course of action would be to send a mail to -devel for the attention of all porters, so that they can have a say about this feature.

I remember that in platforms such as Dreamcast, creating directories was not really possible, as the saved games are saved in the memory card. Could be mistaken, though.

@lolbot-iichan

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2019

This PR is not needed for Hamlet support at Wintermute anymore since we decided not to create directories from engines.
However, replacing create() with a createDirectory() is a good thing, since it makes backend API smaller and more readable, and since noone is actually using create() to create files.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.