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.

Sign up for GitHub

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

Jump to bottom

Add libretro assets directory search path, and have environment variables override configured values #17054

Merged
merged 8 commits into from
Jan 17, 2025
Merged

Add libretro assets directory search path, and have environment variables override configured values #17054

merged 8 commits into from
Jan 17, 2025

Conversation

Apteryks
Copy link
Contributor

Description

See the commit messages, but in a nutshell, this adds support to set/override the assets_directory configured via a LIBRETRO_ASSETS_DIRECTORY environment variable. The behavior of the existing LIBRETRO_DIRECTORY environment variable is changed so that it also overrides any libretro_directory value, with the rationale explained in the commit message.

Related Issues

https://issues.guix.gnu.org/38439

Related Pull Requests

used

@Apteryks
platform_win32: Fix sizeof for DEFAULT_DIR_PLAYLIST.
8f09d98 
* frontend/drivers/platform_win32.c (frontend_win32_env_get): Check
the size of correct destination array.
@Apteryks
frontend: Honor the LIBRETRO_ASSETS_DIRECTORY environment variable.
b5ac44e 
This builds on 763fcd8 ("unix, win32: Allow set the default
libretro_directory via environment variable") to also allow specifying
the assets directory via an environment variable.

* frontend/drivers/platform_unix.c (frontend_unix_get_env)
<libretro_assets_directory> New variable. Use it to set
DEFAULT_DIR_ASSETS, when available.
* frontend/drivers/platform_win32.c (frontend_win32_env_get): Likewise.
@Apteryks Apteryks mentioned this pull request Sep 27, 2024
Merged
@Apteryks
platform: Honor the LIBRETRO_DIRECTORY environment variable.
15a0ff7 
Until now, and unlike the defaut core directory, the default
core *info* directory would be hard-coded relative to the installation
directory. Honor the LIBRETRO_DIRECTORY environment variable the same
instead.

* frontend/drivers/platform_unix.c (frontend_unix_get_env): Set
DEFAULT_DIR_CORE_INFO default to the value of the LIBRETRO_DIRECTORY
environment variable, if available.
* frontend/drivers/platform_win32.c (frontend_win32_env_get): Likewise.
@Apteryks Apteryks force-pushed the add-libretro-assets-directory-search-path branch from 08828a2 to f1df3e7 Compare October 2, 2024 00:45
@Apteryks
platform: Honor the LIBRETRO_AUTOCONFIG_DIRECTORY environment variable.
074dd53 
* frontend/drivers/platform_unix.c
(libretro_autoconfig_directory): New variable.
(frontend_unix_get_env): Set DEFAULT_DIR_AUTOCONFIG to the value of
the LIBRETRO_AUTOCONFIG_DIRECTORY environment variable, if available.
* frontend/drivers/platform_win32.c: Likewise.
@Apteryks
platform: Honor the LIBRETRO_VIDEO_FILTER_DIRECTORY environment varia…
b63cf65 
…ble.

* frontend/drivers/platform_unix.c
(libretro_video_filter_directory): New variable.
(frontend_unix_get_env): Set DEFAULT_DIR_VIDEO_FILTER to the value of
the LIBRETRO_VIDEO_FILTER_DIRECTORY environment variable, if available.
* frontend/drivers/platform_win32.c: Likewise.
@Apteryks
platform: Honor the LIBRETRO_VIDEO_SHADER_DIRECTORY environment varia…
97c87dc 
…ble.

* frontend/drivers/platform_unix.c
(libretro_video_shader_directory): New variable.
(frontend_unix_get_env): Set DEFAULT_DIR_SHADER to the value of
the LIBRETRO_VIDEO_SHADER_DIRECTORY environment variable, if available.
* frontend/drivers/platform_win32.c: Likewise.
@Apteryks Apteryks force-pushed the add-libretro-assets-directory-search-path branch from f1df3e7 to b03edd7 Compare October 2, 2024 03:34
@Apteryks
platform: Honor the LIBRETRO_SYSTEM_DIRECTORY environment variable.
3e7bd0d 
* frontend/drivers/platform_unix.c
(libretro_system_directory): New variable.
(frontend_unix_get_env): Set DEFAULT_DIR_SYSTEM to the value of
the LIBRETRO_SYSTEM_DIRECTORY environment variable, if available.
* frontend/drivers/platform_win32.c: Likewise.
@Apteryks
configuration: Have environment variables override configuration.
9a75152 
Because the configuration file is systematically written when
RetroArch terminates, persisting any previous default/configured
value, setting the LIBRETRO_DIRECTORY, LIBRETRO_ASSETS_DIRECTORY, etc.
environment variables would not have an effect unless the
retroarch.cfg configuration file was cleared.

This seems to go against the common expectation that environment
variables are set by users to *override* the default behavior or
configuration of an application.

* configuration.c (config_load_file) <libretro_directory>
<libretro_assets_directory, libretro_autoconfig_directory>
<libretro_system_directory, libretro_video_filter_directory>
<libretro_video_shader_directory>: New variables. Use the values of
the LIBRETRO_DIRECTORY, LIBRETRO_ASSETS_DIRECTORY,
LIBRETRO_AUTOCONFIG_DIRECTORY, LIBRETRO_SYSTEM_DIRECTORY,
LIBRETRO_VIDEO_FILTER_DIRECTORY and LIBRETRO_VIDEO_SHADER_DIRECTORY
environment variables instead of their corresponding configured
values, when set.
* docs/retroarch.6: Document the environment variables honored and
their behavior.
@Apteryks Apteryks force-pushed the add-libretro-assets-directory-search-path branch from b03edd7 to 9a75152 Compare January 17, 2025 02:30
@JesseTG
Copy link
Contributor

JesseTG commented Jan 17, 2025

Looks good to me. The only thing I'd do is list these new environment variables on the --help listing.

These also look like they'd be helpful for testing and debugging, e.g. so you can set these variables from IDE profiles instead of juggling multiple config files.

@LibretroAdmin LibretroAdmin merged commit 69ceb95 into libretro:master Jan 17, 2025
27 checks passed
@Apteryks
Copy link
Contributor Author

Hey, thanks for reviewing/merging this. I've just found that overriding the database directory is also useful, so I'll send a follow-up PR, tackling your --help suggestion doing so.

@Apteryks Apteryks mentioned this pull request Jan 18, 2025
Merged
@mackal
Copy link

mackal commented Jan 18, 2025

This breaks setting libretro_directory in your cfg file. It keeps resetting to default for me. I DO NOT believe this was the intention since we can still change the path in the UI (which doesn't really persist since it's overwritten with default on next start up)

IMO, ENV should overwrite always (should this save to cfg? maybe, both valid I guess) and then only default if the value in the config is empty. https://github.com/libretro/RetroArch/pull/17054/files#diff-0972109e3e4a166e3d332ecbdecfca1c835cc4e34f2f3e75a577770348e7e9a7L3851-L3856 that's what this removed code was doing I think.

Reverting this commit works as I expect.

@Apteryks
Copy link
Contributor Author

@mackal hello! Uh, sorry for the breakage. Not honoring a non-default libretro_directory from the config file was definitely not the intention. The intention is that LIBRETRO_DIRECTORY should override it, and otherwise leave the existing behavior unchanged. I'll attempt to reproduce your findings.

@mackal
Copy link

mackal commented Jan 19, 2025

@Apteryks remove LIBRETRO_DIRECTORY in from your environment. Set libretro_directory to anything in ~/.config/retroarch/retroarch.cfg (or where ever it is for you) Start Retroarch. Close Retroarch. Check what the setting in the config is now.

@Apteryks
Copy link
Contributor Author

Apteryks commented Jan 19, 2025 •
edited
Loading

Yep, I could reproduce, thank you. Fix is here: 57a6451

I had misread the loop initializing the default value; it has some condition that skips over some configuration keys, and apparently libretro_directory is one of the ones skipped by default (I'm not sure why, the code is https://github.com/libretro/RetroArch/pull/17054/files#diff-0972109e3e4a166e3d332ecbdecfca1c835cc4e34f2f3e75a577770348e7e9a7R3851)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Apteryks @JesseTG @mackal @LibretroAdmin