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

Add GitHub Actions workflow #492

Merged
merged 4 commits into from
Jul 14, 2024
Merged

Conversation

Masterkatze
Copy link
Contributor

Description

  • Add simple GitHub Actions workflow with building for the Windows and Ubuntu operating systems.
  • Add CMake presets.
  • Add separate debug information (.debug-files) support for Linux.
  • Remove BUILD_DATETIME from version info to stop rebuilding if not necessary.
  • Move version info from header to source file.
  • Add CMake install calls (primarily used for GitHub Actions)
  • Add CPack support, although support of it was not needed in the end.
  • Update documentation.

nullsystem
nullsystem previously approved these changes Jul 8, 2024
@nullsystem nullsystem requested a review from a team July 8, 2024 20:10
@xedmain
Copy link
Contributor

xedmain commented Jul 9, 2024

couldn't "RelWithDebInfo" be renamed to just "Release"?

@Masterkatze Masterkatze added Build System CMake, VPC and other build-related stuff CI Continuous Integration (GitHub Actions) labels Jul 9, 2024
@xedmain
Copy link
Contributor

xedmain commented Jul 9, 2024

libraries for both windows and linux seem to still come as bin/* instead of neo/bin/*, in case this isn't intended

@Masterkatze
Copy link
Contributor Author

libraries for both windows and linux seem to still come as bin/* instead of neo/bin/*, in case this isn't intended

That was unintended, thanks for noticing.

@Rainyan
Copy link
Collaborator

Rainyan commented Jul 10, 2024

couldn't "RelWithDebInfo" be renamed to just "Release"?

RelWithDebInfo seems more accurate, since that's what it is.

@xedmain
Copy link
Contributor

xedmain commented Jul 10, 2024

couldn't "RelWithDebInfo" be renamed to just "Release"?

RelWithDebInfo seems more accurate, since that's what it is.

if github actions are gonna be used as a source of getting the latest version of the game for playtesters, having it called "Release" rather than "RelWithDebInfo" seems to be easier to find, even if it's less accurate

@AdamTadeusz
Copy link
Contributor

image
What am I doing wrong

@xedmain
Copy link
Contributor

xedmain commented Jul 10, 2024

how come following cli guide and picking "linux-release" preset built THIS
image
using sniper (steamrt3) docker btw

@xedmain
Copy link
Contributor

xedmain commented Jul 10, 2024

$ file client.so
client.so: ELF 32-bit LSB shared object, Intel 80386, version 1 (SYSV), dynamically linked, BuildID[sha1]=df52aa5838479ec386e4a9c2bf57f55f89d8ba76, with debug_info, not stripped

@Masterkatze
Copy link
Contributor Author

Masterkatze commented Jul 11, 2024

$ file client.so
client.so: ... with debug_info, not stripped

It's correct because (windows/linux)-release profiles are RelWithDebInfo.

I think there is a confusion possible because now these profiles are more suitable for CI rather than developer use. If we want to 1:1 match between profile name and presence/absence of debug info then I think I'll rework this a little, something like this:

  1. With -DNEO_CI_BUILD=OFF (default):
  • linux-debug is Debug, linux-release is Release;
  • -DNEO_USE_SEPARATE_BUILD_INFO=OFF (so you can debug it as usual);
  1. With -DNEO_CI_BUILD=ON`:
  • linux-debug is Debug, linux-release is RelWithDebInfo (renamed to Release for artifact names);
  • -DNEO_USE_SEPARATE_BUILD_INFO=ON (so we can debug crashdumps);

I don't know if we need to add other CMake build types to profiles, if someone need RelWithDebInfo or MinSizeRel then it's still possible to build without profiles (the usual -S and -B).

@xedmain
Copy link
Contributor

xedmain commented Jul 11, 2024

I don't know if we need to add other CMake build types to profiles, if someone need RelWithDebInfo or MinSizeRel then it's still possible to build without profiles (the usual -S and -B).

huh, I thought the presets are like the new, better way to do it replacing the default way. Atleast that's how it looks like, with there not being info on the previous way in the README.md

doesn't "RelWithDebInfo" CI still have the release binaries separated from the debug information? which is why they're ~9mb in size in your actions. That's why I thought it would do the same if I used the "linux-release" preset.

@Masterkatze
Copy link
Contributor Author

doesn't "RelWithDebInfo" CI still have the release binaries separated from the debug information? which is why they're ~9mb in size in your actions. That's why I thought it would do the same if I used the "linux-release" preset.

CI build uses -DNEO_USE_SEPARATE_BUILD_INFO=ON

@xedmain
Copy link
Contributor

xedmain commented Jul 11, 2024

doesn't "RelWithDebInfo" CI still have the release binaries separated from the debug information? which is why they're ~9mb in size in your actions. That's why I thought it would do the same if I used the "linux-release" preset.

CI build uses -DNEO_USE_SEPARATE_BUILD_INFO=ON

I see, I'll try building with that rn.
I'm not sure about for developers, but imo as a playtester that could probably be set by default for release? Unsure what your specific plans are.

@Masterkatze
Copy link
Contributor Author

I'm not sure about for developers, but imo as a playtester that could probably be set by default for release? Unsure what your specific plans are.

I think that playtesters should use the builds only from CI (with separate debug info). Developers can build whatever they want.

@xedmain
Copy link
Contributor

xedmain commented Jul 11, 2024

I'm not sure about for developers, but imo as a playtester that could probably be set by default for release? Unsure what your specific plans are.

I think that playtesters should use the builds only from CI (with separate debug info). Developers can build whatever they want.

I suppose. I guess it being called "release" and having debug info makes sense for developers, considering the differences between the two or something, yeah.
Also -DNEO_USE_SEPARATE_BUILD_INFO=ON worked, neat.

nullsystem
nullsystem previously approved these changes Jul 12, 2024
Some small improvements
@Masterkatze Masterkatze requested a review from a team July 13, 2024 22:51
@Masterkatze Masterkatze merged commit 8621b06 into NeotokyoRebuild:master Jul 14, 2024
5 checks passed
Copy link
Contributor

@brysondev brysondev left a comment

Choose a reason for hiding this comment

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

image
image
lgtm

@Masterkatze Masterkatze deleted the ci branch July 14, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build System CMake, VPC and other build-related stuff CI Continuous Integration (GitHub Actions)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants