tybug / ossapi

The definitive python wrapper for the osu! api.
https://tybug.github.io/ossapi/
GNU Affero General Public License v3.0
82 stars 18 forks source link

Token directory should be created #54

Closed aticie closed 1 year ago

aticie commented 2 years ago
        self.token_directory = (
            Path(token_directory) if token_directory else Path(__file__).parent
        )

Currently if a non-existing token directory is given, this error occurs:

  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\main.py", line 199, in <module>
    main()
  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\main.py", line 94, in main
    api = OssapiV2(client_id=credentials.get("client_id"), client_secret=credentials.get("client_secret"),
  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\venv\lib\site-packages\ossapi\ossapiv2.py", line 294, in __init__
    self.session = self.authenticate()
  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\venv\lib\site-packages\ossapi\ossapiv2.py", line 359, in authenticate
    return self._new_client_grant(self.client_id, self.client_secret)
  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\venv\lib\site-packages\ossapi\ossapiv2.py", line 374, in _new_client_grant
    self._save_token(token)
  File "C:\Users\efeha\PycharmProjects\osu-map-downloader\venv\lib\site-packages\ossapi\ossapiv2.py", line 431, in _save_token
    with open(self.token_file, "wb+") as f:
FileNotFoundError: [Errno 2] No such file or directory: 'abc\\bc7d75f22d1ebc0626b3e9cb595027d58236e659de6cb7801af5533269678079.pickle'

If no token directory is given, this error occurs when an executable is created using PyInstaller because Path(__file__).parent points to a non-existing folder.

tybug commented 2 years ago

what does Path(__file__).parent point to under pyinstaller? I would be slightly concerned about automatically creating directories if it means the standard usage (not passing token_directory) creates a dir in a weird place under pyinstaller.

aticie commented 2 years ago

From what I have encountered, it points to cwd/ossapi, where cwd is the folder of the executable.

aticie commented 2 years ago

Also, what's the concern behind creating directories?

tybug commented 2 years ago

I'd prefer not to touch the user's filesystem under normal usage. I do think it's probably ok to automatically create token_directory for cases when someone passes a nonexistent dir, but if someone _doesn't_ pass token_directory they probably aren't expecting a dir to get created somewhere.

Separately, pyinstaller has previously caused issues as well: https://github.com/sortedcord/osu-statqt#building-osustatqt. We should probably explicitly support pyinstaller's _MEIPASS.

Can you try running pyinstaller with the following patch (untested):

diff --git a/ossapi/ossapiv2.py b/ossapi/ossapiv2.py
index 99a9c5e..3f33adf 100644
--- a/ossapi/ossapiv2.py
+++ b/ossapi/ossapiv2.py
@@ -11,6 +11,7 @@ import inspect
 import json
 import hashlib
 import functools
+import sys

 from requests_oauthlib import OAuth2Session
 from oauthlib.oauth2 import (BackendApplicationClient, TokenExpiredError,
@@ -285,6 +286,10 @@ class OssapiV2:
         self.log = logging.getLogger(__name__)
         self.token_key = token_key or self.gen_token_key(self.grant,
             self.client_id, self.client_secret, self.scopes)
+
+        if hasattr(sys, '_MEIPASS') and not token_directory: # being run from a pyinstall'd app
+            token_directory = Path(sys._MEIPASS) # pylint: disable=no-member
+
         self.token_directory = (
             Path(token_directory) if token_directory else Path(__file__).parent
         )
tybug commented 2 years ago

above patch worked so have applied and released it. Going to sit on whether / how we should be automatically creating token directories for a bit longer.

tybug commented 1 year ago

don't think automatically creating token directories is something I want ossapi to do. Will leave that to consumers to perform if necessary.

Going to close since the root issue of this has, I believe, been solved by supporting _MEIPASS.