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

Fix CloudSync on Windows, enable by default #16475

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KiralyCraft
Copy link
Contributor

@KiralyCraft KiralyCraft commented Apr 28, 2024

Fix Windows CloudSync and enable by default

Related Issues

#6875

Related Pull Requests

#6875

Reviewers

None yet (will edit when ready)

Pending to test

  • Linux/Android/iOS initial sync + Windows MSVC builds (2019, 2022, UWP) target
  • Linux/Android/iOS initial sync + Windows i686 (MXE (MinGW)) target
  • Linux/Android/iOS initial sync + Windows x64 (MXE (MinGW)) target
  • Windows initial sync + Linux/iOS/Android build

@hizzlekizzle
Copy link
Contributor

Hey, just checking in on this. Is there anything you need from our end?

@KiralyCraft
Copy link
Contributor Author

Hi, thanks for checking in. I didn't get to finishing the tests yet, I got stuck at trying to enable OpenSSL for Windows UWP builds. I ran out of time, and put the project on hold for the time being. They have a guide for compilation here: https://github.com/openssl/openssl/blob/master/NOTES-WINDOWS.md

I remember getting stuck at the end, something failed to link (or compile) within OpenSSL itself. Then, there's the matter of how to integrate that into the Visual Studio project in such a way that the "steps" of getting it compiled can be defined and built by the retroarch project itself.

In order to enable SSL for UWP, one would need a way of automating the build of OpenSSL while building retroarch. Windows is not particularly my cup of tea, so this is a bit slower. Although, enabling UWP SSL would also bring Cloud Sync to Xbox platforms, if I'm not mistaken. Maybe, if anyone has an Xbox with UWP, would you mind testing it there as-is, without SSL?

@hizzlekizzle
Copy link
Contributor

Ah, okay, I don't have any xbox myself, but I'll see if I can get some eyes on this. Thanks for the update :)

@LibretroAdmin
Copy link
Contributor

What if we excluded this for now for UWP builds so that we can at least merge this in for the non-UWP versions? Would that be a suitable compromise?

Also, I take it the current implementation and how this works is:

  • The feature is compiled in, but the setting defaults to off and you have to manually enable it, right?

That is the way I'd like to see it implemented at least.

@KiralyCraft
Copy link
Contributor Author

KiralyCraft commented May 19, 2024

That could work, it seems that non-UWP builds work normally. In order to avoid confusion, I would've suggested that all enabling for a platform to be done at once. However, it seems that time is tight, and this feature works quite well as-is.

However, there's still one more thing we could do: Enable this for UWP as well (and Android), but define a flag which, if missing, changes the text associated with Cloud Sync to mention that it's experimental and unencrypted.

This would be needed anyway, since there are some platforms (such as the PS2 and Wii) where the "weight" of an SSL library would probably outweigh the benefits it brings.

The feature is compiled in, but the setting defaults to off and you have to manually enable it, right?

Yes, you have to manually enable and configure it with credentials and a WebDav server, from the "Saving" menu. The reason for concern about SSL is particularly this logging in, which over unencrypted connections makes me feel uneasy.

However, if the user is willing to take the risk, or is providing encryption some other way for the time being (like I am on Android, with a VPN), we might as well let them enable it.

I was thinking of:

  • Using HAVE_SSL to change the label of this feature, to mention it's unencrypted
  • Defining HAVE_CLOUDSYNC_TESTED to hide the fact that the feature is experimental

For example, a build where only HAVE_CLOUDSYNC is enabled would show the feature like Cloud Sync (Unencrypted) (Experimental), whereas an SSL-enabled but untested build would say Cloud Sync (Experimental).

We could probably drop this "tested" flag at some point. Given that this feature actually modifies user data, I would suggest to take this precaution, in order to avoid chaos and data corruption. Would that be alright?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants