gorilla-devs / GDLauncher

GDLauncher is a simple, yet powerful Minecraft custom launcher with a strong focus on the user experience
https://gdevs.io
GNU General Public License v3.0
1.21k stars 254 forks source link

GDLauncher won't recognize Windows' Environment Variables in Java Paths after 1.1.27 update #1430

Open asalimod opened 2 years ago

asalimod commented 2 years ago

Describe the bug GDLauncher 1.1.27 is unable to resolve Java Path if it contains a reference to Windows' Environment Variable.

To Reproduce Steps to reproduce the behavior:

  1. Set new Windows' system variables for different Java runtime installations
  2. Configure Java Paths in GDLauncher 1.1.25 to reference these variables
  3. Create a new instance and launch it, verify that GDLauncher 1.1.25 is able to resolve Java Paths with variable references
  4. Upgrade GDLauncher to 1.1.27 without changing settings
  5. Launch the instance and open DevTools Console
  6. See an Uncaught Error
  7. Try to kill the instance (presumably without success)

Expected behavior The earlier versions of GDLauncher could find an absolute path to Java binaries if the path was relative to some environment variable.

Possible solutions I imagine that the problem is caused by some changes in Node.js, but I can't figure it out.

Screenshots Windows Variables GDLauncher Settings GDLauncher Console

Operating System:

Additional context I have 5 different Java runtime installations used by different programs and Minecraft modpacks. To make managing these runtimes easier (eg. updating, reinstalling, moving, etc.) I set new system variables for each runtime installation and reference these variables instead of absolute paths.

Ladvace commented 2 years ago

could you try instead of %JAVA_SEMERU_17% this $env:JAVA_SEMERU_17 and check fi anything change?

asalimod commented 2 years ago

Unfortunately this doesn't work, I've tried several other variations (like $JAVA_SEMERU_17, ${JAVA_SEMERU_17}, swapping \ and /) but to no avail.

Searching through the code I found that this line passes no-shell option to the Node.js' spawn function on Windows: https://github.com/gorilla-devs/GDLauncher/blob/3a3b4f58cb6474bac2c6ac66c2a947a37888266f/src/common/reducers/actions.js#L3246

I don't know when and why this was changed (as it was before the commit squash), but looking through other peoples' forks I found that it was shell: true in previous versions. Which means that the Windows variables were processed by the Windows' shell before.

According to the Node.js' docs the no-shell option can be more efficient on other platforms but is pretty limiting on Windows. Could it be that there's a typo, and instead of

shell: process.platform !== 'win32'

it should have been this?

shell: process.platform === 'win32'
Eskaan commented 2 years ago

I think it was changed because of the 1.19 Command Line To Long on Windows.

Ladvace commented 2 years ago

I think it was changed because of the 1.19 Command Line To Long on Windows.

yeah, I asked to try that because with shell it use the Powershell or the cmd (depending if it's enabled or not), so the command could not work on one of them

asalimod commented 2 years ago

Oh, I see now. Could it be implemented by a launcher then? I'm not very familiar with JavaScript, but something like this:

function resolveEnvVars(javaPath) {
    if (process.platform === 'win32') {

        let replacements = {}

        for (variable in process.env) {
            const reference = `%${variable}%`;
            if (javaPath.includes(reference)) {
                replacements[reference] = process.env[variable];
            }
        }

        if (Object.keys(replacements).length) {
            let re = new RegExp(Object.keys(replacements).join("|"), "gi");
            return javaPath.replace(re, match => replacements[match]);
        }
    }
    return javaPath;
}
Ladvace commented 2 years ago

could you try this and tell me if it works for you? https://github.com/gorilla-devs/GDLauncher/pull/1433