Compile-time APP_DATA_PATH() and APP_ASSETS_PATH() #3435
Conversation
Willy-JL
requested review from
skotopes,
DrZlo13,
hedger and
gsurkov
as code owners
|
Just occurred to me that this would need an equivalent in ufbt, I don't use that myself so I forgot... I can make a PR there as well if we go through with this PR |
|
Thanks for the PR. Currently, if a .fap file is renamed or copied on the device, it gets a separate app data folder, according to its new file name. That way, you can create multiple isolated environments for a single app. However, there are no apps that would make use of that at the moment - at least to my knowledge. With current implementation from this PR, app is confined to its appid. Surely, that approach has its own benefits and enables a predictable file location. This PR also introduces a system-wide define with "system" name at a fairly low level - maybe its scope could also be reduced to app loader level. I'll look into that and see whether that can be adjusted. |
|
Agreed on the define in As for renaming |
i was looking around to implement this but realized that fap header has app name but not appid... not sure what the best idea would be then... |
|
@Willy-JL Some proxy object that is initialized at the app start and got it name and then all your internal references goes to it. That is not something that should be on firmware side, but something that lives in app bss |
|
so then APP_ASSETS_PATH() and APP_DATA_PATH() will remain half broken? and apps will need to make their own system? or do you mean an alternative way of doing it that is not drop-in, but replaces the existing macros? |
|
ah i think i get what you mean, i was unfamiliar with the concept of .bss... so then, at app launch, loader would initialize this global static variable that is located in bss with the app's id, and the app code references to it? sounds like then we could provide helper macros that construct a path using this variable, but still not handled by firmware. like some sort of printf in app code that uses this appid variable, but inside a macro so app developers dontn eed to worry about the variable itself? would mean this is not strictly drop-in, but close enough, same idea but using a printf or similar under the hood... |
|
yep |
|
im new to those concepts but ill try to figure something out, if you have some examples i could look at that would be greatly appreciated! also i drafted for now, but ill perhaps close this and open a new pr when i have something working |
|
@skotopes i was looking into how to implement this with bss, and had a counter argument. i feel like the behavior of renaming a fap file changing storage location as @hedger described is quite weird. yes it has a very niche use case, but i think it instead leads to a lot more confusion for people messing around with faps installed manually, than the small advantages it provides. instead, i was considering that maybe a better solution would be sticking with what i have in this PR thus far (save for moving where the define is made), so paths are defined at compile time (and backwards compatible with existing code), and instead add a new elf section (or attribute in fap header) for the appid, so that assets extractor doesnt need to rely on fap name and can instead use the same appid that was used at compile time to hardcode the paths. this would fix the disjunction between runtime and compile time paths, as well as as avoid unnecessarily complex logic to store the appid in bss and construct paths on app side at runtime with printf. as well as be backwards compatible, requiring one simple recompile. |
|
FWIW from the user's point of view this PR is worth it even for this
point alone. |
|
Put implementation aside for a moment. Do you guys feel like you want to have ability to have multiple app versions with isolated spaces or not? |
|
While I see the value in that option in some niche use cases, to the average user it can be more of a pain when the app doesn't work as intended when they renamed a file. Say the download a fap, and they download it again on a newer version, but they didn't clean the downloads folder. Now it's also, there's the point that compile time macros are easier to use. No need for furi strings or printf to insert appid at runtime, easier to use, and not a breaking change compared to current api. Personally, I'm 100% for the option of compile time, appid is hardcoded in fap, not chosen by filename. |
|
Also, I recently discovered there is compile time macro "FAP_VERSION". It only makes logical sense to offer "FAP_APPID". I wasn't aware at the time of making this PR, hence the "system" and "FURI_APPID". "FAP_APPID" would work great and be super intuitive, and "APP_DATA_PATH()" can rely on FAP_APPID then? |
There was a problem hiding this comment.
unit tests are failing.
./fbt flash_usb_full FIRMWARE_APP_SET=unit_tests
in cli:
unit_tests / unit_tests test_storage
test_storage_data_path failed:
applications/debug/unit_tests/tests/storage/storage_test.c:468: storage_dir_open(file, "/data")
test_storage_data_path_apps()
test_storage_data_path_apps failed:
applications/debug/unit_tests/tests/storage/storage_test.c:443: 1 expected but was 0
|
Yep, as I mentioned above those tests aren't really relevant anymore. Should I just remove them? |
|
Yes - if they are obsolete. |
|
@hedger done ) |
| furi_string_cat(apps_data_path_with_appsid, furi_thread_get_appid(thread_id)); | ||
| storage_process_common_mkdir(app, apps_data_path_with_appsid); | ||
| if(create_folders && storage_process_common_stat(app, apps_data_appid, NULL) != FSE_OK) { | ||
| FuriString* apps_data = furi_string_alloc_set(STORAGE_APPS_DATA_STEM); |
There was a problem hiding this comment.
we're checking existence of one path, but creating a different one
There was a problem hiding this comment.
it first makes /ext/apps_data here, then 2 lines below it makes /ext/apps_data/appid. it is just ensuring that apps_data exists, previous implementation did this too
|
@hedger unit tests are failing in CI setup step, not in unit test itself it seems. when uploading files for apps_data/nfc, complains that nfc folder already exists, which makes sense because now all literal accesses to /ext/apps_data/anything will create such folder. i guess, update CI to accept "already exist" as success, instead of fail? |
| furi_string_cat(apps_data_path_with_appsid, "/"); | ||
| furi_string_cat(apps_data_path_with_appsid, furi_thread_get_appid(thread_id)); | ||
| storage_process_common_mkdir(app, apps_data_path_with_appsid); | ||
| if(create_folders && storage_process_common_stat(app, apps_data_appid, NULL) != FSE_OK) { |
There was a problem hiding this comment.
This code incorrectly handles paths like "/ext/apps_data/nfc"
See https://github.com/flipperdevices/flipperzero-firmware/actions/runs/10283770489/job/28458336904#step:7:89
There was a problem hiding this comment.
seems like its still the error i mentioned above, setting up the CI unit tests, not running them. code that causes the issue is not in flipper code, but python helper is failing when creating a folder that already exists. i had tried in 887cd4b to make it ignore an "already exists" error but seems like this fix had some syntax issue.
i will fix this, but maybe could consider making it so checking for dir exist in "/ext/apps_data/*" will say that dir already exists?
There was a problem hiding this comment.
I think maintaining compatibility and existing behaviour from API is quite important - looking forward to hearing @DrZlo13's opinion on these changes.
This reverts commit 9d9b029.
|
Need some more work, @hedger will provide details |
looks like he is writing a novel with all the details :P |
|
I think the key points from our past discussion are:
The issue was exposed by unit tests runner script, when modifications were needed to maintain previous behaviour. |
What's new
APP_DATA_PATH()andAPP_ASSETS_PATH()useappidfrom compile time instead of runtimethread_idFAP_APPID, could be useful to app makers aswellVerification
APP_DATA_PATH()orAPP_ASSETS_PATH()in draw/timer callback/ext/apps_.../appid, instead of ending as/ext/apps_.../systemor/ext/apps_.../guiChecklist (For Reviewer)