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

Feature/support for unity licensing server linux #468

Merged
merged 17 commits into from
Oct 22, 2022
Merged

Feature/support for unity licensing server linux #468

merged 17 commits into from
Oct 22, 2022

Conversation

simensan
Copy link
Contributor

Hi,

I needed support for a lot of parallel builds and as such also wanted to use the build server as it unlocks using cheaper licenses that can only be used by CI (https://unity.com/products/unity-build-server). Since game-ci did not currently support this, I have forked and implemented the changes as bare-bones as possible to the level where it currently works for us. Since this might be useful to others I hope to be able to contribute this back to gameci. I am no bash-script-guru, and this was my first time working with TS. So I probably haven't solved it all gracefully, but I am happy to make any fixes/improvements you come up with. I have also only fixed this for the ubuntu platform, but looking at the other platforms they already do not seem to support all features, like using license files. I am happy to try to add this to mac also if desired, but I assume that almost all unity builds will be done on the ubuntu platform to save build-minutes, so I have focused on that initially.

I have added a template for the necessary services-config.json file here: dist/resources/services-config.json.template
I am not sure if there is a better location for it? Alternatively it can just be inlined in some script.

Changes

  • Added UNITY_LICENSING_SERVER build parameter.
  • Added tests to src/model/build-parameters.test.ts for new parameter
  • Clear mocks between tests in src/model/build-parameters.test.ts
  • Fixed cloud runner tests to work with remote repo being a ssh url
  • activate.sh will request a floating license, and return_license.sh will return it.

Checklist

  • Read the contribution guide and accept the code of conduct
  • Readme - not needed
  • Tests - added where I found applicable

@github-actions
Copy link

Cat Gif

Copy link
Member

@webbertakken webbertakken left a comment

Choose a reason for hiding this comment

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

Amazing work!

1 comment. The rest looks great!

Comment on lines 81 to 91
echo "Adding licensing server config"
pushd /resources
cat services-config.json.template | tr -d '\r' | awk "{sub(/%URL%/,\"$UNITY_LICENSING_SERVER\")}1" > services-config.json
mkdir -p /usr/share/unity3d/config/
mv services-config.json /usr/share/unity3d/config/

/opt/unity/Editor/Data/Resources/Licensing/Client/Unity.Licensing.Client --acquire-floating > license.txt #is this accessible in a env variable?
PARSEDFILE=$(grep -oP '\".*?\"' < license.txt | tr -d '"')
export FLOATING_LICENSE
FLOATING_LICENSE=$(sed -n 2p <<< "$PARSEDFILE")
FLOATING_LICENSE_TIMEOUT=$(sed -n 4p <<< "$PARSEDFILE")
Copy link
Member

Choose a reason for hiding this comment

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

The parsing and writing of this file should happen in the logic (javascript) part, not in bash. We're trying to keep the bash part as thin as possible because it's extremely hard to test and maintain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assume you are talking about the services-config.json? I can definitely fix that, but is there any way for me to copy it to the right location inside the container without mounting and doing it from bash? Or should I just mount it to /usr/share/unity3d/config/ in the docker command since I am creating the directory from script anyways?

@simensan
Copy link
Contributor Author

I have moved the processing of services-config file to platform-setup, let me know if you think there is a better place for it.

@webbertakken
Copy link
Member

I have moved the processing of services-config file to platform-setup, let me know if you think there is a better place for it.

Sure! I like that it's part of the logic now. That makes it more manageable for us.
Personally I would argue it's more of a feature setup than platform setup so I would have created a new model for it specifically for this feature. But this is also good for a first iteration.

@simensan
Copy link
Contributor Author

I have moved the processing of services-config file to platform-setup, let me know if you think there is a better place for it.

Sure! I like that it's part of the logic now. That makes it more manageable for us. Personally I would argue it's more of a feature setup than platform setup so I would have created a new model for it specifically for this feature. But this is also good for a first iteration.

I agree, makes more sense. I will make sure to do an iteration on that, will need to make this change to the test runner also, so will do a take on a feature-setup there first.

@webbertakken
Copy link
Member

You have to rebase on main, or merge main into your branch and run yarn build to generate the dist files again. It can't be merged as is.

…inux

# Conflicts:
#	dist/index.js
#	dist/index.js.map
@simensan
Copy link
Contributor Author

Fixed

@codecov-commenter
Copy link

Codecov Report

Merging #468 (7dd9cad) into main (9f79830) will increase coverage by 0.45%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #468      +/-   ##
==========================================
+ Coverage   52.52%   52.98%   +0.45%     
==========================================
  Files          66       66              
  Lines        2056     2059       +3     
  Branches      353      355       +2     
==========================================
+ Hits         1080     1091      +11     
+ Misses        972      964       -8     
  Partials        4        4              
Impacted Files Coverage Δ
src/model/docker.ts 17.24% <ø> (ø)
src/model/image-environment-factory.ts 38.88% <ø> (ø)
src/model/build-parameters.ts 97.72% <100.00%> (+2.37%) ⬆️
src/model/input-readers/git-repo.ts 84.21% <100.00%> (+36.84%) ⬆️
src/model/input.ts 92.71% <100.00%> (+0.09%) ⬆️

@webbertakken webbertakken merged commit 4cb3e59 into game-ci:main Oct 22, 2022
@webbertakken
Copy link
Member

webbertakken commented Oct 22, 2022

Thanks so much for your contribution! 🧡

Please consider (concisely) documenting your feature in the docs @ https://github.com/game-ci/documentation so that other people in the community also know how to use it. People will be able to find your feature because of the search.

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.

3 participants