unix, win32: Allow set the default libretro_directory via environment variable

[ghost]

Hello, this patch allow set libretro_directory via an environment variable LIBRETRO_DIRECTORY.

It's useful for the package manager guix, which dosen't install files under /usr, /lib etc. and disabled the core updater, but have a way to set environment variables to paths in profile (where packages are installed into).
My usage for it: https://issues.guix.gnu.org/46588

[ofry]

@iyzsong This PR has merge conflict. Please fix this.

[ghost]

Rebased and adjust code style, thank you.

[inactive123]

Hi there, sorry for the holdup. I'll ask some other members their thoughts on this.

[jdgleaver]

@iyzsong I'm afraid this is the wrong approach.

• You are calling getenv() in retroarch.c - this is a non-portable function used in code that has to be platform independent
• Setting a hardcoded override like this (which disables any user config) is entirely at odds with the RetroArch way of doing things

If you want to set the default core path based on an environment viable on Unix-based systems, then you will need to do this in RetroArch/frontend/drivers/platform_unix.c - right here:

RetroArch/frontend/drivers/platform_unix.c

Lines 1818 to 1819 in 514534f

	fill_pathname_join(g_defaults.dirs[DEFAULT_DIR_CORE], base_path,
	"cores", sizeof(g_defaults.dirs[DEFAULT_DIR_CORE]));

i.e. g_defaults.dirs[DEFAULT_DIR_CORE] should be overridden if the environment variable LIBRETRO_DIRECTORY is set - in a similar fashion to the way that g_defaults.dirs[DEFAULT_DIR_ASSETS] is set if certain standard directories used by package managers are detected.

[ghost]

@jdgleaver Glad to learn, I'll update this to use platform_unix.c later, thank you!

[ghost]

Code updated to platform_unix.c.

[eadmaster]

why not doing the same on other OSes as well like Windows?

i am prolly going to use this in some scripts.

[inactive123]

Hi, you'd still have to fix this code. This will fail C89 validation right now. Variables need to be declared at either the top of a code function or block.

@jdgleaver Looking past that, what is your general opinion on this PR now? Is it something fit for merge or not?

[jdgleaver]

Aside from the C89 build fix, I would also advise changing:

   if (libretro_directory)
      strlcat(g_defaults.dirs[DEFAULT_DIR_CORE], libretro_directory,
            sizeof(g_defaults.dirs[DEFAULT_DIR_CORE]));

to:

if (!string_is_empty(libretro_directory))
      strlcpy(g_defaults.dirs[DEFAULT_DIR_CORE], libretro_directory,
            sizeof(g_defaults.dirs[DEFAULT_DIR_CORE]));

Other than that, I see no issues with the PR - the essential functionality is fine.

[ofry]

@iyzsong Please fix these nits :)

[ghost]

Fixed, also apply for win32 (not tested).

[jdgleaver]

@iyzsong Apologies, but the change you made here is wrong. All g_defaults.dirs[] values must be set before dir_check_defaults() is called. Could you please put the environment variable override code back in its previous location?

Also, I would appreciate a more descriptive variable name than const char *value :)

It's not an important issue, but it's just good practice to avoid potential variable shadowing (value is such a common name...)

[ofry]

@iyzsong please fix these nits :)

[ghost]

@ofry @jdgleaver Sorry for the delay, should be fixed now :)

[jdgleaver]

@iyzsong Thanks, this looks fine now :)

[ofry]

This fetch translation again???

[ghost]

I know nothing about translation, rebase master again seems fixed that...

[RobLoach]

I like the idea of setting more of the configuration through environment variables.

I could see this being useful outside of just the libretro_directory too. What are your thoughts on applying a similar pattern to other configurations? We could eventually make it all generic config system.

[ofry]

@iyzsong

@DisasterMo DisasterMo 9 days ago

Also: on GitHub, go to your repo's Actions and disable the Crowdin related workflows.

Do it and fix this translation crap again.

[ofry]

@RobLoach Please re-review this.

[gouchi]

It is working if libretro_directory is not defined in retroarch configuration file.

I tested only on Linux with

git fetch origin pull/12040/head:pr12040
git checkout pr12040
./configure && make -j$(nproc)
mkdir /tmp/cores && cp ~/.config/retroarch/cores/* /tmp/cores

export LIBRETRO_DIRECTORY=/tmp/cores && ./retroarch or LIBRETRO_DIRECTORY=/tmp/cores ./retroarch

Main tab > Load Core.

Thank you.

[RobLoach]

It is working if libretro_directory is not defined in retroarch configuration file.

What if it is defined in the configuration?

[gouchi]

Sorry I should have been more clear.

I meant #libretro_directory and if there is something set even libretro_directory = "" it won't work according to my tests.

[ofry]

@iyzsong Please fix this bug in your PR.

[ghost]

Hello, when libretro_directory is already there in the config file, set LIBRETRO_DIRECTORY won't change it.
I think It's the expected behavior, according to @jdgleaver :

Setting a hardcoded override like this (which disables any user config) is entirely at odds with the RetroArch way of doing things

i.e. g_defaults.dirs[DEFAULT_DIR_CORE] should be overridden if the environment variable LIBRETRO_DIRECTORY is set - in a similar fashion to the way that g_defaults.dirs[DEFAULT_DIR_ASSETS] is set if certain standard directories used by package managers are detected.

And set g_defaults.dirs[DEFAULT_DIR_CORE] will only have effect when it's not already set in the config file...

[gouchi]

@iyzsong Thank you for the feedback. So it is working on Linux.

There is a little typo on the title

unix, win32: Allow set the deafult libretro_directory via environment variable

unix, win32: Allow set the **default** libretro_directory via environment variable

If possible somebody should try to make a test on Windows.

[ghost]

@gouchi Ah, good catch, thank you!

[Apteryks]

It seems strange that setting LIBRETRO_DIRECTORY would have zero effect when an existing config file contains already a value for it (which is bound to happen given it's written out every time RetroArch quits).

Could we consider changing its behavior to take precedence over the config file? If the current behavior is desirable somehow, we could introduce a LIBRETRO_DEFAULT_DIRECTORY, or similar?

I was looking into adding a LIBRETRO_ASSETS_DIRECTORY for auto-discovery when using RetroArch with an asset packaging in Guix, and having it written to the retroarch.cfg file is problematic, as it then never gets updated and could go stale.

[RobLoach]

Perhaps introducing a LIBRETRO_DIRECTORY_DEFAULT does make sense in that case. Would this hierarchy make sense?

1. LIBRETRO_DIRECTORY_DEFAULT environment variable
2. libretro_directory config
3. LIBRETRO_DIRECTORY environment variable

[Apteryks]

Unless there's a clear use case for LIBRETRO_DIRECTORY_DEFAULT, I suggest we start with simply modifying LIBRETRO_DIRECTORY (it still works the way it was, but it also overrides any configured value from the config file). See: #17054