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

ci: Add GCC build job for Linux #2027

Merged
merged 14 commits into from
Jan 8, 2025
Merged

Conversation

tomboylover93
Copy link
Contributor

As described in this reply for issue #1994, the idea behind this is to catch compilation errors with GCC, especially with new PRs, so that they can be fixed before being merged. A lot of people on Arch Linux use the shadps4-git PKGBUILD, which does not explicitly use Clang as the compiler and often causes compilation issues. This could also help with code quality as GCC is a more fleshed out compiler and less "tolerant" with errors than Clang.

@squidbus
Copy link
Collaborator

squidbus commented Jan 2, 2025

Personally I don't think we need to add the builds to the release, it will just confuse users with more options and add support burden dealing with more build variants. I think just a job to verify it compiles would be enough, and we can skip packaging and uploading for those jobs and remove from pre-release dependencies.

This also removes the packaging step for linux-sdl-gcc and linux-qt-gcc so that the only available artifacts for download are compiled with Clang
@tomboylover93
Copy link
Contributor Author

Updated accordingly.

@georgemoralis
Copy link
Collaborator

renaming linux builds might break updater , better just use -gcc only and don't add -clang to linux

@tomboylover93
Copy link
Contributor Author

Done that as well.

@MajorP93
Copy link
Contributor

MajorP93 commented Jan 3, 2025

@tomboylover93 @abouvier It does not hurt to check if a PR breaks compilation with GCC but it might also be a good idea for the AUR shadps4-git package to try to stick to the way shadps4 is built for official releases.
Just to streamline things and prevent certain issues from arising.
I mean: the shadps4 Linux compilation guide recommends to use clang for building the project aswell.
Supporting every distribution and tool chain out there is probably out of the scope of this project.

@tomboylover93
Copy link
Contributor Author

I suggested that it be changed to use Clang as well but that's up to them.

@baggins183
Copy link
Contributor

Ive had random problems debugging using gdb/lldb when compiling with clang, and better luck with gcc. So I vote for having a gcc action

@baggins183
Copy link
Contributor

I think just a job to verify it compiles would be enough, and we can skip packaging and uploading for those jobs and remove from pre-release dependencies.

This sounds good to me

@tomboylover93
Copy link
Contributor Author

I believe this is ready for review, both requirements are satisfied (not having a packaging and uploading step for GCC jobs, like a "dry run" of sorts, as well as proper naming for Clang jobs to avoid breaking the updater).

@liberodark
Copy link
Contributor

I think this is good idea.

@georgemoralis
Copy link
Collaborator

something seems to fail here , if you can fix it will go on with merge

@tomboylover93
Copy link
Contributor Author

The error that it gives now that I merged the main branch into it is this:

In file included from /home/runner/work/shadPS4/shadPS4/src/emulator.cpp:19:
/home/runner/work/shadPS4/shadPS4/src/common/elf_info.h:114:40: error: declaration of ‘const Common::PSFAttributes& Common::ElfInfo::PSFAttributes() const’ changes meaning of ‘PSFAttributes’ [-Wchanges-meaning]
  114 |     [[nodiscard]] const PSFAttributes& PSFAttributes() const {
      |                                        ^~~~~~~~~~~~~
/home/runner/work/shadPS4/shadPS4/src/common/elf_info.h:114:25: note: used here to mean ‘union Common::PSFAttributes’
  114 |     [[nodiscard]] const PSFAttributes& PSFAttributes() const {
      |                         ^~~~~~~~~~~~~
/home/runner/work/shadPS4/shadPS4/src/common/elf_info.h:20:7: note: declared here
   20 | union PSFAttributes {
      |       ^~~~~~~~~~~~~

which fixing it is out of scope for this PR and way beyond my C++ knowledge. But it's a good thing that it's there because errors like this are why I made this PR.

@georgemoralis
Copy link
Collaborator

error should be fixed , if you can rebase we can check

@tomboylover93
Copy link
Contributor Author

GCC checks still fail.

@ngoquang2708
Copy link
Contributor

GCC version in CI runner is too old?

@ngoquang2708
Copy link
Contributor

ngoquang2708 commented Jan 7, 2025

-- The CXX compiler identification is GNU 13.3.0
-- The C compiler identification is GNU 13.3.0

Default is 13, there is 14 in universe repo but I don't know if GH CI enable that repo or not.

@tomboylover93
Copy link
Contributor Author

I probably need to specify gcc-14 and g++-14 as the compilers instead of just gcc and g++.

@ngoquang2708
Copy link
Contributor

Ah, I see you already install 14.

@tomboylover93
Copy link
Contributor Author

Now it fails in a dependency:

/home/runner/work/shadPS4/shadPS4/externals/discord-rpc/thirdparty/rapidjson-1.1.0/include/rapidjson/document.h: In member function ‘rapidjson::GenericStringRef<CharType>& rapidjson::GenericStringRef<CharType>::operator=(const rapidjson::GenericStringRef<CharType>&)’:
/home/runner/work/shadPS4/shadPS4/externals/discord-rpc/thirdparty/rapidjson-1.1.0/include/rapidjson/document.h:319:82: error: assignment of read-only member ‘rapidjson::GenericStringRef<CharType>::length’
  319 |     GenericStringRef& operator=(const GenericStringRef& rhs) { s = rhs.s; length = rhs.length; }
      |                                                                           ~~~~~~~^~~~~~~~~~~~
At global scope:
cc1plus: note: unrecognized command-line option ‘-Wno-global-constructors’ may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option ‘-Wno-exit-time-destructors’ may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option ‘-Wno-covered-switch-default’ may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option ‘-Wno-c++98-compat-pedantic’ may have been intended to silence earlier diagnostics
cc1plus: note: unrecognized command-line option ‘-Wno-c++98-compat’ may have been intended to silence earlier diagnostics
gmake[2]: *** [externals/discord-rpc/src/CMakeFiles/discord-rpc.dir/build.make:79: externals/discord-rpc/src/CMakeFiles/discord-rpc.dir/discord_rpc.cpp.o] Error 1
gmake[1]: *** [CMakeFiles/Makefile2:2077: externals/discord-rpc/src/CMakeFiles/discord-rpc.dir/all] Error 2
gmake[1]: *** Waiting for unfinished jobs....

@ngoquang2708
Copy link
Contributor

Same error osmcode/osmium-tool#276.

@ngoquang2708
Copy link
Contributor

ngoquang2708 commented Jan 7, 2025

You can skip building rapidjson by install it from Ubuntu repo.

@ngoquang2708
Copy link
Contributor

@ngoquang2708
Copy link
Contributor

I use Arch which doesn't have a -dev package. Sorry about that.

@tomboylover93
Copy link
Contributor Author

I looked at Launchpad before adding that and it just said rapidjson which I thought was an actual package. Should work now.

@ngoquang2708
Copy link
Contributor

Hmm, It would still fail if that only a header lib 😭

@tomboylover93
Copy link
Contributor Author

I tried looking for a .deb file for the actual rapidjson package so I could set it up to download it with curl and install it, but I haven't found any.

@ngoquang2708
Copy link
Contributor

Been wondering why I can build with Arch's rapidjson. Its because Arch patched it.
https://gitlab.archlinux.org/archlinux/packaging/packages/rapidjson/-/commit/20e6c15a0fd8276e9c9200e337db6374695dc68b.

@squidbus
Copy link
Collaborator

squidbus commented Jan 7, 2025

Fixing up the discord-rpc rapidjson here: shadps4-emu/ext-discord-rpc#1

@georgemoralis
Copy link
Collaborator

Merged if you can ,update the submodule in the branch to check

@tomboylover93
Copy link
Contributor Author

Both Qt and SDL GCC jobs pass now.

@georgemoralis georgemoralis merged commit 0eee36c into shadps4-emu:main Jan 8, 2025
12 checks passed
@tomboylover93 tomboylover93 deleted the gcc-ci branch January 8, 2025 13:10
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.

7 participants