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.

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

Support activeBuildProfile parameter #674

Open
Sammmte opened this issue Oct 3, 2024 · 13 comments
Open

Support activeBuildProfile parameter #674

Sammmte opened this issue Oct 3, 2024 · 13 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@Sammmte
Copy link

Sammmte commented Oct 3, 2024

Context

When using "activeBuildProfile" parameter (in "customParameters", example: customParameters: -activeBuildProfile Assets/Settings/BuildProfiles/WindowsDevelopment.asset) (for Unity 6) builder action throws an error message:

Aborting batchmode due to failure:
Can't have -activeBuildProfile argument defined when using -buildTarget argument

Problem is "targetPlatform" is a required parameter.

Suggested solution

Maybe require to have either "targetPlatform" parameter OR another parameter like "buildProfile". "buildProfile" parameter would hold the same value "activeBuildProfile" requires, a relative path to the profile asset.

@Sammmte Sammmte added the enhancement New feature or request label Oct 3, 2024
@GabLeRoux
Copy link
Member

GabLeRoux commented Oct 3, 2024

Build Profiles is a relatively new feature and would be a great addition. 👍

More details about the build profiles

image

@GabLeRoux GabLeRoux added the help wanted Extra attention is needed label Oct 3, 2024
@OwenSignaturize
Copy link

This would be incredibly useful

@GabLeRoux
Copy link
Member

@coderabbitai read the following files and suggest an approach to add support for activeBuildProfile parameter for unity version >= 6 (6000)

dist/default-build-script/Assets/Editor/UnityBuilderAction/Builder.cs
dist/default-build-script/Assets/Editor/UnityBuilderAction/Versioning/GitException.cs
src/index.ts
src/model/platform-setup/setup-windows.ts
src/model/unity.test.ts
src/model/input.ts
dist/default-build-script/Assets/Editor/UnityBuilderAction/Versioning/VersionApplicator.cs
src/model/action.ts
dist/default-build-script/Assets/Editor/UnityBuilderAction/Versioning/Git.cs
dist/default-build-script/Assets/Editor/UnityBuilderAction/Versioning/VersionGenerator.cs
src/model/action.test.ts
src/model/unity.ts
dist/default-build-script/Assets/Editor/UnityBuilderAction/Input/ArgumentsParser.cs
dist/default-build-script/Assets/Editor/UnityBuilderAction/Reporting/StdOutReporter.cs
dist/default-build-script/Assets/Editor/UnityBuilderAction/Reporting/CompileListener.cs
src/model/input.test.ts
src/model/platform-setup/setup-mac.ts
src/model/platform-setup/setup-android.ts
src/model/docker.ts
src/model/docker.test.ts
action.yml

Copy link

coderabbitai bot commented Dec 5, 2024

Tip

For best results, initiate chat on the files or code changes.

Based on the files analyzed, I'll suggest an approach to add support for the activeBuildProfile parameter for Unity version >= 6. Here's what needs to be changed:

@GabLeRoux Here's the proposed approach:

  1. First, add a new optional input parameter in action.yml:
  buildProfile:
    required: false
    default: ''
    description: 'Path to the Unity Build Profile asset file. When specified, targetPlatform will be ignored as the build target is defined in the profile.'
  1. Update src/model/input.ts to include the new parameter:
static get buildProfile(): string {
  return Input.getInput('buildProfile') ?? '';
}
  1. Modify the validation logic in dist/default-build-script/Assets/Editor/UnityBuilderAction/Input/ArgumentsParser.cs to make buildTarget optional when buildProfile is specified:
public static Dictionary<string, string> GetValidatedOptions()
{
    Dictionary<string, string> validatedOptions;
    ParseCommandLineArguments(out validatedOptions);

    string projectPath;
    if (!validatedOptions.TryGetValue("projectPath", out projectPath)) {
        Console.WriteLine("Missing argument -projectPath");
        EditorApplication.Exit(110);
    }

    // Check if buildProfile is specified
    string buildProfile;
    bool hasBuildProfile = validatedOptions.TryGetValue("buildProfile", out buildProfile) && !string.IsNullOrEmpty(buildProfile);

    // Only require buildTarget if buildProfile is not specified
    if (!hasBuildProfile)
    {
        string buildTarget;
        if (!validatedOptions.TryGetValue("buildTarget", out buildTarget)) {
            Console.WriteLine("Missing argument -buildTarget (required when buildProfile is not specified)");
            EditorApplication.Exit(120);
        }

        if (!Enum.IsDefined(typeof(BuildTarget), buildTarget)) {
            Console.WriteLine(buildTarget + " is not a defined " + typeof(BuildTarget).Name);
            EditorApplication.Exit(121);
        }
    }

    // Rest of the validation...
}
  1. Update dist/default-build-script/Assets/Editor/UnityBuilderAction/Builder.cs to handle the build profile:
public static void BuildProject()
{
    // Gather values from args
    var options = ArgumentsParser.GetValidatedOptions();

    // Gather values from project
    var scenes = EditorBuildSettings.scenes.Where(scene => scene.enabled).Select(s => s.path).ToArray();
    
    // Get all buildOptions from options
    BuildOptions buildOptions = BuildOptions.None;
    foreach (string buildOptionString in Enum.GetNames(typeof(BuildOptions))) {
        if (options.ContainsKey(buildOptionString)) {
            BuildOptions buildOptionEnum = (BuildOptions) Enum.Parse(typeof(BuildOptions), buildOptionString);
            buildOptions |= buildOptionEnum;
        }
    }

    // Define BuildPlayer Options
    var buildPlayerOptions = new BuildPlayerOptions {
        scenes = scenes,
        locationPathName = options["customBuildPath"],
        options = buildOptions,
    };

    // If buildProfile is specified, add it to customParameters
    if (options.TryGetValue("buildProfile", out var buildProfile) && !string.IsNullOrEmpty(buildProfile))
    {
        if (options.ContainsKey("customParameters"))
        {
            options["customParameters"] += $" -activeBuildProfile {buildProfile}";
        }
        else
        {
            options["customParameters"] = $"-activeBuildProfile {buildProfile}";
        }
    }
    else
    {
        // Use traditional buildTarget approach
        buildPlayerOptions.target = (BuildTarget)Enum.Parse(typeof(BuildTarget), options["buildTarget"]);
    }

    // Rest of the build process...
}
  1. Update the documentation to explain the new parameter and its usage:
## Using Build Profiles (Unity 6+)

For Unity 6 and above, you can use Build Profiles instead of specifying a target platform. Build Profiles provide more control over your build settings and can be managed through Unity Editor.

Example workflow:
```yaml
- uses: game-ci/unity-builder@v4
  with:
    buildProfile: 'Assets/Settings/BuildProfiles/WindowsDevelopment.asset'
    buildName: 'My Game'
    buildsPath: build

Note: When using buildProfile, the targetPlatform parameter will be ignored as the build target is defined in the profile.


This approach:
1. Makes `targetPlatform` optional when `buildProfile` is specified
2. Passes the build profile path through the `-activeBuildProfile` parameter to Unity
3. Maintains backward compatibility for projects not using build profiles
4. Provides clear documentation for users

Would you like me to proceed with creating a GitHub issue to track this enhancement?



<details>
<summary>🔗 Analysis chain</summary>

Let me gather the necessary information from the files to suggest an approach for adding support for the activeBuildProfile parameter.



</details>

<details>
<summary>🏁 Scripts executed</summary>

The following scripts were executed for the analysis:

Script:

#!/bin/bash

First, let's check the action.yml to see the current input parameters

echo "=== action.yml ==="
cat action.yml

Let's look at the input handling in TypeScript

echo -e "\n=== src/model/input.ts ==="
cat src/model/input.ts

Check how build arguments are constructed

echo -e "\n=== src/model/unity.ts ==="
cat src/model/unity.ts

Check the Builder.cs which handles the actual build

echo -e "\n=== dist/default-build-script/Assets/Editor/UnityBuilderAction/Builder.cs ==="
cat "dist/default-build-script/Assets/Editor/UnityBuilderAction/Builder.cs"

Check how arguments are parsed

echo -e "\n=== dist/default-build-script/Assets/Editor/UnityBuilderAction/Input/ArgumentsParser.cs ==="
cat "dist/default-build-script/Assets/Editor/UnityBuilderAction/Input/ArgumentsParser.cs"


Length of output: 25578

</details>

---

<details>
<summary>✏️ Learnings added</summary>

Learnt from: GabLeRoux
URL: #674

Timestamp: 2024-12-05T17:12:45.657Z
Learning: For Unity version 6 and above, the -activeBuildProfile parameter cannot be used together with -buildTarget parameter as they are mutually exclusive. Build profiles are a new feature that encapsulate build settings including the target platform.


> Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure [review instructions](https://docs.coderabbit.ai/guides/review-instructions). You can manage existing learnings in the [app](https://app.coderabbit.ai/learnings).

</details>

@webbertakken
Copy link
Member

Is there a CLI flag for it? If yes, then that's probably preferable over modifying the build script itself.

If no, then this looks exactly how we should solve it.

Happy to take a PR for this!

@GabLeRoux
Copy link
Member

@coderabbitai There is an existing command line argument for this on the unity cli, it is -activeBuildProfile. Here is an example of how I used it in a different project (this is a confirmed to be working solution used with GitlabCI, not the GameCI Unity Builder GitHub Action):

#!/usr/bin/env bash

set -euo pipefail

echo "Building for '$BUILD_TARGET'"

# Ensure the build path is defined and exists
export BUILD_PATH="$UNITY_DIR/Builds/$BUILD_TARGET/"
mkdir -p "$BUILD_PATH"

# Base Unity command split into an array
mapfile -t UNITY_CMD < <(
  printf "%s\n" \
    "${UNITY_EXECUTABLE:-xvfb-run}" \
    "--auto-servernum" \
    "--server-args=-screen 0 640x480x24" \
    "unity-editor"
)

# Base Unity arguments
mapfile -t UNITY_ARGS < <(
  printf "%s\n" \
    "-projectPath" "$UNITY_DIR" \
    "-quit" \
    "-batchmode" \
    "-nographics" \
    "-customBuildTarget" "$BUILD_TARGET" \
    "-customBuildName" "$BUILD_NAME" \
    "-customBuildPath" "$BUILD_PATH" \
    "-executeMethod" "BuildCommand.PerformBuild" \
    "-logFile" "/dev/stdout"
)

# Adjust arguments based on whether a build profile is provided
if [[ -n "${BUILD_PROFILE_PATH:-}" && -f "$BUILD_PROFILE_PATH" ]]; then
  echo "Using build profile: '$BUILD_PROFILE_PATH'"
  UNITY_ARGS+=("-activeBuildProfile" "$BUILD_PROFILE_PATH")
else
  echo "No build profile specified or the file does not exist; proceeding with default build target."
  UNITY_ARGS+=("-buildTarget" "$BUILD_TARGET")
fi

# Print arguments for debugging
echo "Unity Command: ${UNITY_CMD[*]}"
echo "Unity Arguments: ${UNITY_ARGS[*]}"

# Run the Unity command with arguments
"${UNITY_CMD[@]}" "${UNITY_ARGS[@]}"

UNITY_EXIT_CODE=$?

# Check the Unity exit code and display appropriate messages
if [[ $UNITY_EXIT_CODE -eq 0 ]]; then
  echo "Build succeeded with no failures."
elif [[ $UNITY_EXIT_CODE -eq 2 ]]; then
  echo "Build succeeded, but some tests failed."
elif [[ $UNITY_EXIT_CODE -eq 3 ]]; then
  echo "Build failed due to other reasons."
else
  echo "Unexpected Unity exit code: $UNITY_EXIT_CODE."
fi

# List the build output directory contents
ls -la "$BUILD_PATH"

# Fail the job if the build folder is empty
if [[ -z "$(ls -A "$BUILD_PATH")" ]]; then
  echo "Build folder is empty! Failing the job."
  exit 1
fi

Here are some additional details I am aware of:

  1. It is important to not pass a full path or even a relative path starting with ./ to the -activeBuildProfile parameter. It must use a local path to the asset file. For exmaple:
BUILD_PROFILE_PATH: "Assets/Settings/Build Profiles/Android Mobile.asset"
BUILD_PROFILE_PATH: "Assets/Settings/Build Profiles/Android VR Oculus.asset"
  1. The documentation for the -activeBuildProfile is available here: https://docs.unity3d.com/Manual/EditorCommandLineArguments.html
Command Description
-activeBuildProfile <pathname> Set the build profile saved at the given path as an active build profile (e.g., -activeBuildProfile "Assets/Settings/Build Profiles/WindowsDemo.asset"). Applies custom scripting defines found in the Build data section of the active build profile before compilation. These scripting defines are additive and don’t override other scripting defines in your project.

@webbertakken
Copy link
Member

Impressive @GabLeRoux :)

This comment was marked as off-topic.

@webbertakken
Copy link
Member

@GabLeRoux Looks like the formatting of that last message is a bit off towards the end. The solution also doesn't look viable.

@coderabbitai So we're not going to talk about how to get those new kinds of arguments inside of javascript will make it across the boundary of docker and be usable inside the bash script that's run inside the container?

This comment was marked as off-topic.

@webbertakken
Copy link
Member

Ended up hiding those comments as they add a stream of information that isn't really helpful.

For whoever feels like taking a stab at this, please have a look at the combination of the first comment from Coderabbat and the followup from @GabLeRoux.

@JanikHelbig-NB
Copy link

I've looked a bit at this already and so far the biggest issue (I think) has not been mentioned.
(Which is why I think that AI assistants are mostly useless, surface-level noise; but whatever)

The action currently does a bit of build platform specific setup depending on the build target. If the build profile were to be specified instead of the build target, the necessary information would not be available at the required time.

We could try to extract the build target information from the BuildProfile asset file directly and use that to fill in the action parameter. That way we wouldn't need to change most of the existing code. I'm just not sure how robust and reliable that would be.

@webbertakken
Copy link
Member

We're just casually trialing this bot. It's been doing some very helpful comments as well.

Agreed that auto-detect would be preferable where possible. We already do that for the unityVersion or editorVersion param and could do the same for this.

That said, I think the whole point of Build Profiles is to have multiple. So beyond auto-detecting we do need to be able to specify it as a param as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants