-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Cloud storage #11582
Cloud storage #11582
Conversation
This pull request introduces 1 alert when merging fe991ce into 0d2b9fe - view on LGTM.com new alerts:
|
Hi there, thanks so much for this PR! I will try getting some eyeballs on this PR so we can review it. |
This pull request introduces 1 alert when merging 1b670e6 into 0a3306a - view on LGTM.com new alerts:
|
Hi there @njordan64 , just letting you know that once @jdgleaver has wrapped up some of the work he is doing right now, he will be going over this PR as well. It's a big PR so it might take us some time to fully review this but rest assured we have all intent in the world to merge this. Thanks a lot for your work and patience, we'll get it over the finish line. |
Thanks for the update. It's good to see that there is interest in this feature. |
@njordan64 Thank you for the PR - this is very interesting functionality. Before we continue, however, could I please ask you to fix all the build errors and warnings? I started doing this myself, but there are too many of them. You can see the issues by building as follows:
After that, one immediate thing that needs fixing is the segfault when enabling cloud storage via the menu. I believe this diff is sufficient: diff --git a/cloud-storage/cloud_storage.c b/cloud-storage/cloud_storage.c
index 8b2074f4d2..4e15a8e348 100644
--- a/cloud-storage/cloud_storage.c
+++ b/cloud-storage/cloud_storage.c
@@ -186,6 +186,9 @@ void cloud_storage_init(void)
settings_t *settings;
struct _operation_queue_item *sync_item;
+ if (!_shutdown)
+ return;
+
_providers[0] = cloud_storage_google_create();
_providers[1] = cloud_storage_onedrive_create();
_providers[2] = NULL;
@@ -1003,4 +1006,4 @@ bool cloud_storage_need_authorization(void)
void cloud_storage_authorize(void (*callback)(bool success))
{
_get_active_provider()->authorize(callback);
-}
\ No newline at end of file
+}
diff --git a/menu/menu_setting.c b/menu/menu_setting.c
index 79d3680adb..c17734332d 100644
--- a/menu/menu_setting.c
+++ b/menu/menu_setting.c
@@ -126,6 +126,10 @@
#include "../play_feature_delivery/play_feature_delivery.h"
#endif
+#if defined(HAVE_NETWORKING) && defined(HAVE_CLOUD_STORAGE)
+#include "../cloud-storage/cloud_storage.h"
+#endif
+
#define _3_SECONDS 3000000
#define _6_SECONDS 6000000
#define _9_SECONDS 9000000
@@ -7513,6 +7517,15 @@ static void general_write_handler(rarch_setting_t *setting)
}
}
break;
+#if defined(HAVE_NETWORKING) && defined(HAVE_CLOUD_STORAGE)
+ case MENU_ENUM_LABEL_CLOUD_STORAGE_ENABLE:
+ if (*setting->value.target.boolean)
+ cloud_storage_init();
+ else
+ cloud_storage_shutdown();
+
+ break;
+#endif
default:
break;
} Following this, could you please finish wiring up the cloud storage configuration menu, because at present it is entirely non-functional - and I can't really test the code properly if I cannot configure anything :) |
I'll take a look at your diff and fix up the C89 issues. |
I was able to start with a configuration file that had nothing configured for cloud-storage and enable then authorize with Google. These are the steps that I follow.
I don't have any idea what is the best way of handling the client ID and client secret at this point. I doubt the average user will create a Google/MS API project, but one is needed to access Google Drive/OneDrive. These settings could be added to the menu, although they feel too low level to me. Another problem with an API project is that it could be billed for API usage. Unfortunately this means that the RetroArch project can't just provide one even if the client secret could somehow be kept secret. |
This pull request introduces 2 alerts when merging 5bcb485 into 8921d31 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging b8e47a4 into 8921d31 - view on LGTM.com new alerts:
|
@njordan64 Thank for implementing the fixes. Could you provide a little more information on setting this up? I created an OAuth 2.0 credential, updated the retroarch config, but all I get is this:
I have no idea what I'm supposed to do.
Yes, this is going to be a real problem. I suppose the ID could be entered by selecting the JSON credentials file downloaded from the APIs & Services dashboard (i.e. just open an in-menu file browser). But from my experience thus far, this entire configuration procedure will be entirely inaccessible for 99% of our users. I'm not trying to be negative in any way here - just raising the point that all of us need to think about how this will work in practice, and that the user interface really should take priority. I don't have any ideas yet, but will give this some consideration... |
When you create the credentials, create them as Desktop application credentials. It looks possible that you created them as something else like Web Application. |
I'll try a few things over the weekend to see if there any other ways we can authenticate with Google and hopefully MS. It may be possible to simplify things for users. |
I did some more research on this and we likely are in better shape than I first thought, at least for Google Drive. Google doesn't bill for Google Drive API requests. They make money on Google Drive through the storage. There are API quotas, but those can be increased by submitting requests. This means that the RetroArch project can provide an API Project (with client ID and secret) and user's use this API project. A Google account will be needed and the API quotas will likely need to be increased. There are other open source projects that include the client secret. With RClone, they discussed the issue and it would be very similar to RetroArch in this case. Worst case, someone takes the client ID and secret to impersonate RetroArch. This leads to two possibilities:
In either case, the impersonating application is only able to read and write the RetroArch application data in the user's Google Drive account. |
It looks like MS OneDrive is similar to Google Drive. The API is free to use. MS makes money from the storage. MS OneDrive only requires a client ID, but in reality this is basically the same as Google Drive along with the same issues. MS OneDrive also allows the API project to be configured to a limited set of APIs (just like Google Drive). |
This is very encouraging news! Looking at RClone as an example, I guess what it will be doing internally is the same setup as described here: https://rclone.org/drive/, only with as much autoconfigured as possible. So the authorisation menu can have client ID/secret entries that are blank by default, with a sublabel recommendation that users provide their own for better performance, but otherwise internal RetroArch credentials will be used. This seems highly practical. Can you update the PR with a proof of concept of this? (Absolutely no hurry, of course - you're in the driving seat here) I'm certain @twinaphex can look into setting up an API Project on RetroArch's behalf. |
I'm working on an update that supports default credentials. Should be ready in a day or two. |
Excellent! We very much appreciate your work. And please take all the time you need! A day or two or more - it's all fine. There are no deadlines here :) |
Now this supports default client ID/secrets. There are constants in google.c and onedrive.c that can hold the default client ID/secret. I have left them blank for now. If you populate those, it will be possible to choose to use default credentials in the configuration menu. If they are empty, then you cannot choose to use defaults and must provide the client ID/secret. There were also some issues with reading the authorization request from the browser. I have improved this somewhat, but it probably still needs some work. |
This pull request introduces 1 alert when merging 849a3d5 into 4dcc556 - view on LGTM.com new alerts:
|
Thank you so much for working on this, @njordan64. As an enthusiast user who has been relying on Dropbox for syncing their RA save files, I am really excited at the prospect of having this functionality baked in RA and hopefully being able to use it from all sorts of devices that lack Cloud support at an OS level (namely consoles and handhelds). I would like to share here one feature request. Do you think it's possible to implement cloud sync support also for the "playtime logs" folder and maybe for the screenshots as well? The logs would be particularly nice for maintaining a global per-game playtime count across multiple devices. I guess that when the infrastructure is laid out fully this should be feasible, but I am no programmer, so I'm voicing this as purely a practical suggestion based on my personal use case. Looking forward to seeing how this pans out. |
@njordan64 Thank you for these new additions! Much appreciated :) We're currently a bit snowed under with the transition to the new build server, but we'll start testing this as soon as possible. @twinaphex We're going to need default client ID/secrets for Google Drive and OneDrive, so this will need to be done under 'official' RetroArch accounts. Could you maybe look into this at some point...? (I don't think anyone else has the authority...) |
I'll take a look at including the playtime logs and screenshots folders. On devices without a browser, this will get a little ugly thanks to OAuth 2.0. Everything aside from the authorization flow would work fine. Basically RetroArch could only display a URL that you need to open in a browser on a different device and then enter the refresh code into RetroArch. The URL is probably around 200 characters long (a URL shortener or QR code could help) and the refresh token is about 100 characters long. Could you see yourself entering the refresh code on a device like a Switch or PSP? |
This pull request introduces 1 alert when merging 743a8f6 into fe430fd - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 5efaad2 into cf854fc - view on LGTM.com new alerts:
|
Is there any possibility on local SMB sync feature using this as a basse? |
SMB is wildly different from REST APIs. Currently I don't see SMB fitting in with the cloud-storage syncing. There are ways of syncing over SMB using features within your operating system. If there is a lot of desire for SMB support, then it could be explored. I'd like to wait until cloud-storage support is merged in and stable first though. |
This pull request introduces 1 alert when merging d6ee556 into 4d2eb18 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 79d5a3f into 4d2eb18 - view on LGTM.com new alerts:
|
This pull request introduces 1 alert when merging 64366c3 into 34190cc - view on LGTM.com new alerts:
|
…cify credentials, or use defaults. Added support for default credentials. Fixed up some issues with authorization.
…nd writing to a file for a response body.
…ixed up reading of the HTTP request.
…ors in sending HTTP data from a file. Fixed up a null pointer error when creating a remote folder.
Fixed up issues with freeing already freed memory. Added settings for using default credentials.
This pull request introduces 1 alert when merging 2c6f5c5 into a967d22 - view on LGTM.com new alerts:
|
This is likely quite stale at this point. I'll rebase in case anyone wants to test it out. |
This patch is unworkable. |
Description
Adds support for syncing the game saves and game states directories with a storage in a cloud provider. Currently supports Google Drive (App Storage) and MS OneDrive (App Storage).
At a high level the following changes were made:
HTTP Changes
HTTP Request
Contains the following:
Related functions:
net_http_request_new
- Creates a blank HTTP requestnet_http_request_set_url
- Set the HTTP URLnet_http_request_set_method
- Set the HTTP methodnet_http_request_set_url_param
- Add or replace an HTTP URL parameternet_http_request_set_header
- Add or replace an HTTP headernet_http_request_set_body
- Set the request body and lengthnet_http_request_free
- Free the memory for a requestHTTP Response
Contains the following:
Related functions:
net_http_response_get_status
- Get the HTTP statusnet_http_response_is_error
- If the request failednet_http_response_get_header_first_value
- Get the first value for a header namenet_http_response_get_header_values
- Get all values for a header namenet_http_response_get_data
- Get the response body and lengthnet_http_response_release_data
- Release the memory for the response bodynet_http_response_free
- Release the memory for the responseHow the REST requests are executed
When the cloud storage support is initialized during startup, it will start up a new thread. There is a queue where cloud storage operations (sync and upload) can be pushed to. When the thread is free, it will pull tasks from the queue and execute them. When possible, it will try to avoid overlapping operations.
New Configuration Items
cloud_storage_enable
- boolean - Cloud storage support is enabled if truecloud_storage_google_client_id
- String - The Google client ID to usecloud_storage_google_client_secret
- String - The Google client secret to usecloud_storage_google_refresh_token
- String - The refresh token. Can be obtained using RetroArch using the cloud storage configuration menucloud_storage_onedrive_client_id
- String - The MS OneDrive client ID to usecloud_storage_onedrive_refresh_token
- String - The refresh token. Can be obtained using RetroArch using the cloud storage configuration menucloud_storage_provider
- int - The cloud storage provider to use. 0 is Google Drive, 1 is MS OneDrive.Configuration Menu For Cloud Storage
In the cloud storage configuration menu, you can choose a provider, set the client ID and client secret (if needed). You can then choose to authorize RetroArch, which will follow the OAuth2 flow for authorization and open the authorization page in your browser. After you have authorized RetroArch, it will update the refresh token configuration and sync files based on your configured game saves and game states directories.
What It Actually Does
Assuming that everything is configured, it will do the following:
Limitations
Further Work Needed