Skip to content

WIP: Improve CI setup#10

Open
bbodenmiller wants to merge 138 commits into
JonathanPorta:masterfrom
bbodenmiller:patch-2
Open

WIP: Improve CI setup#10
bbodenmiller wants to merge 138 commits into
JonathanPorta:masterfrom
bbodenmiller:patch-2

Conversation

@bbodenmiller

@bbodenmiller bbodenmiller commented Mar 13, 2017

Copy link
Copy Markdown

DO NOT MERGE YET... working on getting it all fixed up.

Improve CI setup. Build is still failing but not as bad as before. Current error is RenderTexture.Create failed: cubemap not supported. but it works on local machine so not sure what is going wrong.

Similar project at https://github.com/SebastianJay/unity-ci-test has working CI project. yourdatasucks/Fresh-Unity-Travis-CI#1 has same error we do.

Fix #2 and #6

@JonathanPorta

Copy link
Copy Markdown
Owner

PR Review

Summary

This PR is a 2017 WIP branch that rewrites the Unity CI setup across Travis, AppVeyor, Unity project assets, and shell wrappers. The PR body explicitly says “DO NOT MERGE YET” and notes the build is still failing. Given its age and the experimental commit history, this should be closed and replaced with a fresh CI modernization branch if the goal is still relevant.

Findings

  • Severity: Blocking
    Location: PR status / .travis.yml / Unity project changes
    Problem: The PR is explicitly marked “DO NOT MERGE YET” and says the build is still failing with RenderTexture.Create failed: cubemap not supported. The branch also includes many exploratory commits and Unity project rewrites from 2017.
    Why it matters: Merging this would land a known-failing CI setup and a stale Unity project migration rather than a validated improvement.
    Suggested fix: Close this PR. Start a current branch against the present repo state with a supported Unity version, current CI provider, and a green build before review.

  • Severity: Important
    Location: .travis.yml deploy config
    Problem: The Travis S3 deploy block hardcodes an AWS access key ID (AKIA...) in the workflow.
    Why it matters: Even when the secret key is encrypted, committing access key identifiers is unnecessary exposure and ties the repo to an old credential. For a stale branch, it is also unclear whether that key is still valid or rotated.
    Suggested fix: Remove committed AWS key IDs from CI config and move deploy credentials fully into the CI secret store. Rotate the key if it was ever active.

  • Severity: Important
    Location: Scripts/unity_stdout.sh and Scripts/install.sh
    Problem: The scripts use eval to expand the Unity path and run installer commands (eval printf $UNITY, eval "\"$UNITY\" $@ ...", eval ./... /S).
    Why it matters: These scripts execute values that can come from environment variables or downloaded filenames, which is risky in CI and brittle for paths containing shell metacharacters.
    Suggested fix: Use arrays/quoted arguments instead of eval, and validate downloaded installer filenames before execution.

  • Severity: Minor
    Location: Unity assets/project settings
    Problem: git diff --check reports trailing whitespace across the new script asset and many Unity settings files.
    Why it matters: It is not the main blocker, but it confirms this branch still needs cleanup before it could be considered merge-ready.
    Suggested fix: Let the current Unity/editor tooling regenerate clean project settings in a fresh branch.

Test / Verification Notes

I inspected the PR metadata/body, comments, changed file list, Travis/AppVeyor configs, installer/build/stdout scripts, Unity project version, added behavior script, and project settings diff.

Commands run:

  • bash -n Scripts/install.sh Scripts/build.sh Scripts/unity_stdout.sh — passed.
  • Ruby YAML parsing for .travis.yml and appveyor.yml — passed.
  • git diff --check origin/master...HEAD — failed on trailing whitespace in Unity assets/settings.

I did not run Unity/AppVeyor/Travis builds locally because this requires old Unity 5.x installers and CI environments, and the PR body already documents a failing build.

Overall Recommendation

Needs changes before merge.

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.

Unity 5.3 installer onward cannot be used

2 participants