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: Remove the directory creation functionality #1787

Closed
wants to merge 1 commit into from

Conversation

@bluegr
Copy link
Member

commented Aug 4, 2019

It's a poorly supported feature (only 3 backend file system implementations support it, and 8 don't), and it's only used in two places in the codebase, where it can be disabled.

This is a different approach than the one taken in pull request #1727: the directory creation feature is removed, as it's poorly supported in the different file system implementations (only 3 support it, and 8 don't)

BACKENDS: Remove the directory creation functionality
It's a poorly supported feature (only 3 backend file system
implementations support it, and 8 don't), and it's only used in two
places in the codebase, where it can be disabled.
@Tkachov

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

Uh... did you try downloading a game from cloud storage with these changes? I think it might not work since it looks like DownloadRequest depends on the fact that DumpFile creates intermediate directories (asking user to create all subdirectories manually it too much).

Plus don't know if that's convinient for user that when using local webserver you can't create a directory to put your files into.

Finally, it looks like most necessary -- Windows and POSIX -- filesystems are supported. Why can't we ask port maintainers to implement mkdir() for their systems?

@bluegr

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

@Tkachov: No, I did not try the cloud storage changes. But a different approach could be used in this case (e.g. create temporary files with prefixes).

The problem is, that at the moment this feature isn't really supported in our backends, and if we take into account the low rate that changes are done to backends, implementing this could take awhile.

Thus, we should either choose to remove this feature altogether, or ask porters to implement it, and limit its usage in the meantime to backend code only. For backend features like cloud support this could work, but it's certainly not doable to use this feature in engine code, in its current implementation state.

@ccawley2011

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

I'd prefer it if the directory creation code were retained. The main reason why I opened PR #1727 was to clean up how creating the default save path is created, so that all the other cases where directories are created in the backend code can be removed, and can instead be handled in a common location, as is partially done in commit aa86c51.

The problem is, that at the moment this feature isn't really supported in our backends, and if we take into account the low rate that changes are done to backends, implementing this could take awhile.

I can potentially provide implementations for the remaining platforms, however it shouldn't be a major issue if it can't be implemented on a certain platform, as in that case, the function should just return an error code which can be handled by the calling code if needed.

@Tkachov

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

@bluegr: Sorry, I still don't follow.

  1. Why exactly is it a problem that feature is implemented and works, but not on all platforms? Why is it better to remove feature at all than to wait (even for a while) until porters implement it?

  2. It's not ready to be used in engines, but do any engines need to create directories? If not, why is that a problem again? As you said, we can limit usage of this feature to backend (i.e. cloud/local webserver) only, so basically just leave it as it is.


Meanwhile, I tested it: as expected, neither downloading works, nor local webserver directory creation.

BTW, I don't see how temporary files with prefixes is a solution. Let's say I'm a user and I want to play the game on my PC and my Android phone when I'm AFK. I've got game files on my PC, and I thought it's easier to upload it to Dropbox and then download it via ScummVM than moving it via USB cable.

Let's say this game has "data" directory. So, I use ScummVM to download the game, and it creates "data__file" instead of "data/file". Even if the game is detected correctly, it will crash when I run it, because engine does not expect that there are no directories. Does it mean that I, as a user, should go and create all directories myself, rename all files and move them into these directories? Because if ScummVM can't create directories, how else will prefixes turn into directories?

@bluegr

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

  1. Why exactly is it a problem that feature is implemented and works, but not on all platforms? Why is it better to remove feature at all than to wait (even for a while) until porters implement it?

Because if a game relies on this partially implemented feature, we lose portability

  1. It's not ready to be used in engines, but do any engines need to create directories? If not, why is that a problem again? As you said, we can limit usage of this feature to backend (i.e. cloud/local webserver) only, so basically just leave it as it is.

Yes, but we should enforce this rule, like what was done with the Wintermute engine

Meanwhile, I tested it: as expected, neither downloading works, nor local webserver directory creation.

Fair enough. So at least the cloud saves feature needs this

@bluegr

This comment has been minimized.

Copy link
Member Author

commented Aug 4, 2019

I can potentially provide implementations for the remaining platforms, however it shouldn't be a major issue if it can't be implemented on a certain platform, as in that case, the function should just return an error code which can be handled by the calling code if needed.

Sure, if you could do that, it would help a lot. The reason for opening this PR was to attract some attention and reach a solution

@sev-

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

The proper way would be to implement it, or if it is not possible, the define as a backend feature and query it before usage.

So, the functionality needs to stay.

@sev- sev- closed this Aug 4, 2019

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