Skip to content

Expand environment strings on Windows (allow %temp% in paths), and use system temp directory on Windows for Cache#6482

Open
Dwedit wants to merge 2 commits intolibretro:masterfrom
Dwedit:system_temp_directory
Open

Expand environment strings on Windows (allow %temp% in paths), and use system temp directory on Windows for Cache#6482
Dwedit wants to merge 2 commits intolibretro:masterfrom
Dwedit:system_temp_directory

Conversation

@Dwedit
Copy link
Copy Markdown
Contributor

@Dwedit Dwedit commented Mar 28, 2018

Description

This is a change for RetroArch to allow use of environment variables (such as %temp%) in path names on Windows.
The default Cache directory is also now set on Windows to be "%temp%/retroarch_temp/", rather than defaulting to the RetroArch directory.
On a typical system, %temp% is "C:\users\username\appdata\local\temp"

Use of the %temp% environment variable vs a call to get the system temp directory allows the same installation to be used by multiple users without temp files getting created into another user's temp directory.

Related Issues

Fixes #6438, and addresses part of #6458

Dwedit added 2 commits March 28, 2018 11:35
…mp% = C:\users\userna~1\Appdata\Local\Temp)

Replace most uses of utf8_to_utf16_string_alloc with utf8_to_utf16_string_alloc_expand_environment_strings
@inactive123
Copy link
Copy Markdown
Contributor

Hi there, looks like a good addition.

Since this is touching upon UTF8 codeparts, I will invite @bparker06 into the conversation and let him quickly review the changes made to this.

@ghost
Copy link
Copy Markdown

ghost commented Mar 28, 2018

I will need some time to test this, especially with non-ascii language systems and on older Windows versions like 9x. Also, 9x versions' %TEMP% resolves to a system dir, so in this case it might actually be a step backwards, as the original RetroArch/system dir at least could differ from one user to another if installed in different places.

@andres-asm
Copy link
Copy Markdown
Contributor

andres-asm commented Mar 28, 2018

I think the RA installer package should use the platform defined storage locations, so RA should be installed in Program Files like a regular app and the default dirs would default to dirs inside appdata.

That said, a portable mode (like the one we have now) should remain, the usual method is having a portable.ini file alongside with the RA executable.

I usually use a userdata dir inside my RA dir for my data.

All this being said. This has to be tested very thoroughly imho.

@ghost
Copy link
Copy Markdown

ghost commented Mar 28, 2018

@fr500 The installer would be totally unrelated to this PR, we currently use the default user storage location, which is inside AppData on Vista and up. If we had an option to "install for all users" then we could have that default to Program Files like you said.

@andres-asm
Copy link
Copy Markdown
Contributor

Yeah that's actually what I mean, install for all users would use program files for the exe, the dependencies, and hard assets that never need updating (I don't think we have any of those)

Then on first startup defaults would be set to %APPDATA%/Local/RetroArch/

Whatever happens, please retain the ability to have a portable installation

@ghost
Copy link
Copy Markdown

ghost commented Mar 29, 2018

@Dwedit Well the first thing I noticed is that it doesn't seem to work :/

Thread 1 hit Breakpoint 2, 0x00007fff5b56a7a0 in msvcrt!_wfopen ()
   from C:\WINDOWS\System32\msvcrt.dll
(gdb) up
#1  0x0000000000424e91 in fopen_utf8 (
    filename=0x5af9c50 "%temp%/retroarch_temp/Donkey Kong Country (USA).sfc",
    mode=0x97116e <vfs_error_return_value+78> "wb")
    at libretro-common/compat/fopen_utf8.c:32
32         FILE* ret = _wfopen(filename_w, mode_w);

This is on a Windows 10 FCU system with CP932 locale (but the value of TEMP does not contain any non-ascii chars). The %temp% does not seem to have expanded at all. I haven't yet tested any other Windows versions, wanted to make sure it worked on the latest OS first.

Also I think we should not be modifying people's existing cache folder settings as this seems to do... a new default is one thing (and @twinaphex still needs to be ok with that), but some people expect the cache to be in a specific place and we shouldn't mess with that if it has already been set. It also shows up in the menu with the wrong slashes, e.g. all the other paths on that page use a \ while this uses / and also has a trailing slash as well whereas the others don't.

Thoughts?

@Dwedit
Copy link
Copy Markdown
Contributor Author

Dwedit commented Mar 29, 2018

Currently, on the shipping version of RetroArch, the default Cache directory is Blank, meaning store stuff in the main RetroArch folder (not a subdirectory). The change was to change the default Blank value into something else. If a value is configured, it won't change.

And that's weird that the path expansion would fail, it's expanding the paths fine on my system. (CP932 code page system) When I stick debugger breakpoints in MSVC, it shows the expanded paths in memory. Preprocessor issues? Does it even execute ExpandEnvironmentStringsW?

Wrong slashes? Okay, that's a cosmetic issue.... Win32 allows you to use either kind of slash interchangeably. I wasn't sure whether I needed a terminating slash or not. Easy to change that.

@ghost
Copy link
Copy Markdown

ghost commented Mar 30, 2018

This is strange, I repeated the same test on the same machine and now it's working...

Thread 1 hit Breakpoint 6, utf8_to_utf16_string_alloc_expand_environment_strings (str=0x5af9cc0 "%temp%/retroarch_temp/Donkey Kong Country (USA).sfc")
    at libretro-common/encodings/encoding_utf.c:431
431           requiredSize = ExpandEnvironmentStringsW(result, NULL, 0);
(gdb) n
432           if (requiredSize == 0)
(gdb) print requiredSize
$1 = 59
(gdb) n
436           bufferSize = (int)(requiredSize + 1);
(gdb)
437           result2 = (wchar_t*)malloc(bufferSize * sizeof(wchar_t));
(gdb)

Thread 1 hit Breakpoint 7, utf8_to_utf16_string_alloc_expand_environment_strings (str=0x5af9cc0 "%temp%/retroarch_temp/Donkey Kong Country (USA).sfc")
    at libretro-common/encodings/encoding_utf.c:438
438           ExpandEnvironmentStringsW(result, result2, bufferSize);
(gdb) print result
$2 = 0x15447dd0 L"%temp%/retroarch_temp/Donkey Kong Country (USA).sfc"
(gdb) n
439           free(result);
(gdb) print result2
$4 = 0x15447e70 L"C:\\msys64\\tmp/retroarch_temp/Donkey Kong Country (USA).sfc"

I'll test some MSVC builds and other Windows versions next and see if anything comes up.

@ofry
Copy link
Copy Markdown

ofry commented Mar 4, 2019

This PR has conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants