Force module install on Mac #659
Conversation
|
|
|
I'm slightly confused, does the main Unity Editor install function not install all the necessary modules already? It looks like the ensure function that's been added is doing the same install process but using the install-modules command instead. |
|
From what I can see, this installs Unity and the module twice if it wasn't already installed. Usually you'd expect ephemeral runners. I'm not sure if this PR's solution to making sure the right module is installed in case of a non ephemeral runner is comprehensive enough. At the very least we must verify that this doesn't run the install twice for users that do not have this use case. |
📝 WalkthroughWalkthroughThe changes introduce a new method, Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
dist/.DS_Storeis excluded by!**/dist/**,!**/.DS_Storedist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (1)
- src/model/platform-setup/setup-mac.ts (2 hunks)
🧰 Additional context used
🪛 Biome
src/model/platform-setup/setup-mac.ts
[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)
| await SetupMac.ensureRequiredModuleIsInstalled(buildParameters); | ||
|
|
There was a problem hiding this comment.
🛠️ 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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
| '--childModules', | ||
| ]; | ||
|
|
||
| await exec(this.unityHubExecPath, execArguments, { |
There was a problem hiding this comment.
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.
| 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)
| await exec(this.unityHubExecPath, execArguments, { | ||
| silent: true, | ||
| ignoreReturnCode: true, | ||
| }); |
There was a problem hiding this comment.
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.
| 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)
There was a problem hiding this comment.
Author of #615 here. Yes, this situation is encountered when on non-ephemeral runners.
The code in this PR is pretty much exactly my suggested code, and I have tested the Unity Hub commands in this scenario. Running unityhub -- --headless install-modules with a unity editor version that is already installed will not reinstall the editor, and if the module is already installed, it will exit with an error and not reinstall the module(s).
Changes
Related Issues
On Mac, Unity is installed automatically with the right module.
But if Unity is already installed with the wrong module on the machine, it will run without installing the missing module.
Successful Workflow Run Link
PRs don't have access to secrets so you will need to provide a link to a successful run of the workflows from your own
repo.
Checklist
code of conduct
in the documentation repo)
Summary by CodeRabbit