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/windows upgrades #588

Merged
merged 26 commits into from
Oct 28, 2023
Merged

Feature/windows upgrades #588

merged 26 commits into from
Oct 28, 2023

Conversation

AndrewKahr
Copy link
Member

@AndrewKahr AndrewKahr commented Oct 27, 2023

Changes

  • Allow updating container memory and cpu limits for Windows. Previously, they defaulted to 1cpu and 1gb ram which was far too low and it seems docker wouldn't allocate all available resources. Now it will use all available cores and 80% of system memory.
  • Allow setting docker isolation mode for windows. Defaults to default to ensure behavior doesn't change from prior versions but now you can do stuff like force process mode on non-server versions which grants a performance uplift during runs
  • Added logic to allow building Android on Windows. Android doesn't support burst when built on Linux, only on Windows and macOS. Thus we need to allow building Android on WIndows due to the major performance benefits of Burst.
  • Support Windows 2022 and VS2022 by mounting the x64 Visual Studio path in addition to the x86 path to maintain compatibility with VS2019 and older
  • Attempted fixes for windows builds hanging by killing the regsvr32 process after registering VS dll and using a different method to launch Unity. Unsure if this is a definite fix so I am leaving in several debug calls to print out running processes so we have more data to work with on chasing down this bug. I suspect there's a process that's hanging around that isn't cleaning itself up or is getting into some kind of deadlock situation and needs to be killed. But the changes I've made have seen no hangs on building during docker test workflows when previously there would be at least 3-5 hanging builds.

Related PRs

This PR also needs game-ci/documentation#433 to be merged to document the new parameter inputs. Then we will be able to work on merging a PR for the updated docker images to go along with this PR

Related Issues

Checklist

  • Read the contribution guide and accept the
    code of conduct
  • Docs (If new inputs or outputs have been added or changes to behavior that should be documented. Please make
    a PR in the documentation repo)
  • Readme (updated or not needed)
  • Tests (added, updated or not needed)

@github-actions
Copy link

Cat Gif

@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2023

Codecov Report

Merging #588 (37dc04e) into main (6419c87) will increase coverage by 0.19%.
The diff coverage is 66.66%.

❗ Current head 37dc04e differs from pull request most recent head 12c712d. Consider uploading reports for the commit 12c712d to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #588      +/-   ##
==========================================
+ Coverage   37.02%   37.22%   +0.19%     
==========================================
  Files          77       77              
  Lines        3041     3060      +19     
  Branches      642      649       +7     
==========================================
+ Hits         1126     1139      +13     
- Misses       1765     1771       +6     
  Partials      150      150              
Files Coverage Δ
src/model/build-parameters.ts 89.39% <100.00%> (ø)
src/model/docker.ts 10.63% <0.00%> (ø)
src/model/input.ts 87.94% <68.42%> (-3.05%) ⬇️

action.yml Show resolved Hide resolved
@AndrewKahr AndrewKahr merged commit 4c4611c into main Oct 28, 2023
33 checks passed
@AndrewKahr AndrewKahr deleted the feature/windows-upgrades branch October 28, 2023 19:21
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