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

Force module install on Mac #659

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Force module install on Mac #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified dist/.DS_Store
View file
Open in desktop
Binary file not shown.
18 changes: 18 additions & 0 deletions dist/index.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion dist/index.js.map
View file
Open in desktop

Large diffs are not rendered by default.

22 changes: 22 additions & 0 deletions src/model/platform-setup/setup-mac.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ class SetupMac {
await SetupMac.installUnity(buildParameters);
}

await SetupMac.ensureRequiredModuleIsInstalled(buildParameters);

Comment on lines +23 to +24
Copy link

@coderabbitai coderabbitai bot Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Avoid redundant module installations by checking if the module is already installed

Unconditionally calling ensureRequiredModuleIsInstalled may lead to reinstalling modules that are already present, increasing setup time and potentially causing issues on non-ephemeral runners. Consider implementing a check to determine if the required module for the target platform is already installed before attempting installation.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea AI. How would one do this? I do not know of any way to get Unity Hub to expose what modules are currently installed other than asking to install them.

Docs can be found here: https://docs.unity3d.com/hub/manual/HubCLI.html

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rycieos, thanks for pointing that out. The script didn't work because the UNITY_VERSION and BUILD_TARGET environment variables were empty. We can address this by integrating the check directly into the ensureRequiredModuleIsInstalled method. Here's how you might modify the method to check if the required module is already installed before attempting installation:

private static async ensureRequiredModuleIsInstalled(buildParameters: BuildParameters) {
  const unityEditorPath = `/Applications/Unity/Hub/Editor/${buildParameters.editorVersion}/Unity.app/Contents`;
  let modulePath = '';

  switch (buildParameters.targetPlatform) {
    case 'iOS':
      modulePath = `${unityEditorPath}/PlaybackEngines/iOSSupport`;
      break;
    case 'Android':
      modulePath = `${unityEditorPath}/PlaybackEngines/AndroidPlayer`;
      break;
    case 'StandaloneOSX':
      modulePath = `${unityEditorPath}/PlaybackEngines/MacStandaloneSupport`;
      break;
    case 'WebGL':
      modulePath = `${unityEditorPath}/PlaybackEngines/WebGLSupport`;
      break;
    default:
      throw new Error(`Unsupported target platform: ${buildParameters.targetPlatform}.`);
  }

  if (fs.existsSync(modulePath)) {
    console.log(`Module for ${buildParameters.targetPlatform} is already installed.`);
  } else {
    console.log(`Module for ${buildParameters.targetPlatform} is not installed. Installing now...`);
    const unityChangeset = await getUnityChangeset(buildParameters.editorVersion);
    const moduleArguments = SetupMac.getModuleParametersForTargetPlatform(buildParameters.targetPlatform);

    const execArguments: string[] = [
      '--',
      '--headless',
      'install-modules',
      ...['--version', buildParameters.editorVersion],
      ...['--changeset', unityChangeset.changeset],
      ...moduleArguments,
      '--childModules',
    ];

    await exec(this.unityHubExecPath, execArguments, {
      silent: true,
      ignoreReturnCode: true,
    });
  }
}

By checking if the module's directory exists using fs.existsSync(modulePath), we can determine whether the required module is already installed. If the module is missing, the script proceeds to install it using Unity Hub CLI.

Let me know if this solution works for you!

(•ᴗ•)

🔗 Analysis chain

@Rycieos, you're correct that Unity Hub CLI doesn't provide a direct way to list installed modules. However, we can check if the required module's files exist in the Unity installation directory before attempting installation. Here's a script that performs this check:

This script checks if the module directory exists within the Unity installation. If the directory is not found, it proceeds to install the module using Unity Hub CLI. Integrating this check into the ensureRequiredModuleIsInstalled method should prevent redundant installations.

(•ᵕ•)

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if the required Unity module is installed on macOS

UNITY_VERSION="${UNITY_VERSION}"
BUILD_TARGET="${BUILD_TARGET}"
UNITY_EDITOR_PATH="/Applications/Unity/Hub/Editor/${UNITY_VERSION}/Unity.app/Contents"
UNITY_HUB_EXEC_PATH="/Applications/Unity Hub.app/Contents/MacOS/Unity Hub"

case "${BUILD_TARGET}" in
  iOS)
    MODULE_PATH="${UNITY_EDITOR_PATH}/PlaybackEngines/iOSSupport"
    MODULE_NAME="ios"
    ;;
  Android)
    MODULE_PATH="${UNITY_EDITOR_PATH}/PlaybackEngines/AndroidPlayer"
    MODULE_NAME="android"
    ;;
  StandaloneOSX)
    MODULE_PATH="${UNITY_EDITOR_PATH}/PlaybackEngines/MacStandaloneSupport"
    MODULE_NAME="mac-il2cpp"
    ;;
  WebGL)
    MODULE_PATH="${UNITY_EDITOR_PATH}/PlaybackEngines/WebGLSupport"
    MODULE_NAME="webgl"
    ;;
  *)
    echo "Unsupported target platform: ${BUILD_TARGET}"
    exit 1
    ;;
esac

if [ -d "${MODULE_PATH}" ]; then
  echo "Module for ${BUILD_TARGET} is already installed."
else
  echo "Module for ${BUILD_TARGET} is not installed."
  # Proceed to install the module
  "${UNITY_HUB_EXEC_PATH}" -- --headless install-modules --version "${UNITY_VERSION}" --module "${MODULE_NAME}" --childModules
fi

Length of output: 286

await SetupMac.setEnvironmentVariables(buildParameters, actionFolder);
}

Expand Down Expand Up @@ -119,6 +121,26 @@ class SetupMac {
return moduleArgument;
}

private static async ensureRequiredModuleIsInstalled(buildParameters: BuildParameters) {
const unityChangeset = await getUnityChangeset(buildParameters.editorVersion);
const moduleArguments = SetupMac.getModuleParametersForTargetPlatform(buildParameters.targetPlatform);

const execArguments: string[] = [
'--',
'--headless',
'install-modules',
...['--version', buildParameters.editorVersion],
...['--changeset', unityChangeset.changeset],
...moduleArguments,
'--childModules',
];

await exec(this.unityHubExecPath, execArguments, {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Replace 'this' with class name in static method

Using this in a static method can be confusing since this refers to the class, not an instance. Replace this.unityHubExecPath with SetupMac.unityHubExecPath for clarity.

Apply this diff to fix the issue:

-        await exec(this.unityHubExecPath, execArguments, {
+        await exec(SetupMac.unityHubExecPath, execArguments, {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await exec(this.unityHubExecPath, execArguments, {
await exec(SetupMac.unityHubExecPath, execArguments, {
🧰 Tools
🪛 Biome

[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

silent: true,
ignoreReturnCode: true,
});
Comment on lines +138 to +141
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Handle the error code from 'exec' to detect installation failures

Currently, the exec call ignores the return code and does not handle errors, which may lead to silent failures during module installation. Consider capturing the error code and throwing an error if the installation fails, similar to the approach used in installUnityHub.

Apply this diff to handle the error code:

-        await exec(SetupMac.unityHubExecPath, execArguments, {
-          silent: true,
-          ignoreReturnCode: true,
-        });
+        const errorCode = await exec(SetupMac.unityHubExecPath, execArguments, {
+          silent: true,
+          ignoreReturnCode: true,
+        });
+        if (errorCode) {
+          throw new Error(`There was an error installing the required module. See logs above for details.`);
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
await exec(this.unityHubExecPath, execArguments, {
silent: true,
ignoreReturnCode: true,
});
const errorCode = await exec(SetupMac.unityHubExecPath, execArguments, {
silent: true,
ignoreReturnCode: true,
});
if (errorCode) {
throw new Error(`There was an error installing the required module. See logs above for details.`);
}
🧰 Tools
🪛 Biome

[error] 138-138: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

}

private static async installUnity(buildParameters: BuildParameters, silent = false) {
const unityEditorPath = `/Applications/Unity/Hub/Editor/${buildParameters.editorVersion}`;
const key = `Cache-MacOS-UnityEditor-With-Module-${buildParameters.targetPlatform}@${buildParameters.editorVersion}`;
Expand Down
Loading