-
Notifications
You must be signed in to change notification settings - Fork 800
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
update xonotic example to 0.8.6 #3273
update xonotic example to 0.8.6 #3273
Conversation
Build Succeeded 👏 Build Id: 1d3557da-a691-434b-b3e1-40c5523da559 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
We had a chat offline, but just wanted to confirm that with the changes both the Linux and Windows image build? |
Linux image build working as expected but I am not able to build Windows image due to system issue. |
0e2a9e7
to
76f0346
Compare
Build Failed 😱 Build Id: e93dd269-abdb-458f-a25c-ff8669c76d6b To get permission to view the Cloud Build view, join the agones-discuss Google Group. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first glance - this look great! Just running a local test as well to double check everything runs as expected.
Only extra thing, we'll need to update the push
target to build the multi-arch manifest.
You can use the simple-game-server as example:
agones/examples/simple-game-server/Makefile
Lines 78 to 87 in 4d1086f
push: push-linux-image-amd64 | |
ifeq ($(WITH_WINDOWS), 1) | |
push: push-windows-images | |
endif | |
ifeq ($(WITH_ARM64), 1) | |
push: push-linux-image-arm64 | |
endif | |
-docker manifest rm $(server_tag) | |
docker manifest create $(server_tag) $(push_server_manifest) | |
docker manifest push $(server_tag) --purge |
Take a look, but definitely let me know if you have any questions on the above.
76f0346
to
5d576d6
Compare
Build Succeeded 👏 Build Id: f1aa6419-df2f-4a76-83cc-d08dcd224507 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
examples/xonotic/Makefile
Outdated
ifeq ($(WITH_WINDOWS), 1) | ||
push_server_manifest += $(foreach windows_version, $(WINDOWS_VERSIONS), $(server_tag)-windows_amd64-$(windows_version)) | ||
endif | ||
ifeq ($(WITH_ARM64), 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just double checked: https://xonotic.org/download/
And Xonotic doesn't support Arm, only x86-64 - so let's remove the arm support please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just double checked: https://xonotic.org/download/
And Xonotic doesn't support Arm, only x86-64 - so let's remove the arm support please.
Sure, I will remove it.
examples/xonotic/Makefile
Outdated
ifeq ($(WITH_WINDOWS), 1) | ||
server_tag_linux_amd64 = $(server_tag)-linux-amd64 | ||
server_tag_linux_arm64 = $(server_tag)-linux-arm64 | ||
else | ||
server_tag_linux_amd64 = $(server_tag)-amd64 | ||
server_tag_linux_arm64 = $(server_tag)-arm64 | ||
endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ifeq ($(WITH_WINDOWS), 1) | |
server_tag_linux_amd64 = $(server_tag)-linux-amd64 | |
server_tag_linux_arm64 = $(server_tag)-linux-arm64 | |
else | |
server_tag_linux_amd64 = $(server_tag)-amd64 | |
server_tag_linux_arm64 = $(server_tag)-arm64 | |
endif | |
server_tag_linux_amd64 = $(server_tag)-linux-amd64 | |
server_tag_linux_arm64 = $(server_tag)-linux-arm64 |
I know we do this everywhere, but I don't know why, so let's break the habit and remove the conditional. It's not really doing anything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we do this everywhere, but I don't know why, so let's break the habit and remove the conditional. It's not really doing anything.
Even, I was confused why we are doing this everywhere. But, At the end I had to follow the coding guidelines (Current codebase written in this way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
server_tag_linux_arm64 = $(server_tag)-linux-arm64
I think, this is not required. Since, Xonotic does not have support for ARM64, I am removing everthing related to ARM64 in makefile.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tried to run this but ran into an issue - I expect it's because I ran with WITH_ARM=0
, so will try again once the ARM support is gone, I expect it will resolve the issue.
5d576d6
to
59814a4
Compare
Build Succeeded 👏 Build Id: a4a8ecef-25d2-45a1-902d-c6a0d208b871 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just ran this locally - and looks good!
Only thing left to do, is change the image to 1.2
in all the yaml files in https://github.com/googleforgames/agones/tree/main/examples/xonotic and we can merge this in!
59814a4
to
f81cc71
Compare
All yaml file are having |
Build Succeeded 👏 Build Id: 817e3102-4691-4384-acb6-f8bbf3ddfd75 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aimuz, ashutosji, markmandel The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #3233
Special notes for your reviewer: