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

build: Build system overhaul #1686

Merged
merged 15 commits into from
Dec 31, 2024
Merged

build: Build system overhaul #1686

merged 15 commits into from
Dec 31, 2024

Conversation

jcm93
Copy link
Contributor

@jcm93 jcm93 commented Oct 31, 2024

This PR introduces a new CMake-based build system for ares.

Broadly, the goals of this build system are to:

  • Modernize the build process somewhat, and make ares more approachable for new developers
  • Improve ares's dependency handling
  • Expand and standardize platform/compiler/architecture support for ares
  • Improve project code quality
  • Improve build performance
  • Lay groundwork for easier support of more platforms in the future

There are a few notable changes that this PR brings about. Briefly:

  • Dynamic dependencies (librashader, MoltenVK, SDL, slang-shaders) are no longer built in-tree
  • Build system generation and building now take place as two separate steps

Other notes:

  • The legacy build system is deprecated, but is planned to still function until such time as it is removed in a later PR

New Features

  • ares can now generate projects for Xcode and Visual Studio 17 2022. Xcode is the recommended generator on macOS, supporting universal binaries, advanced profiling and GPU debugging. Visual Studio projects can be configured with either Clang-CL or MSVC, and may be generated for either x86_64 or arm64.

  • On macOS and Windows, dynamic dependencies are now built by ares-deps and fetched at configure-time. This is to reduce build requirements for development, define explicit versioning for dependencies, reduce build times generally, and streamline CI workflows.

  • Builds can now be configured through the default Windows command line rather than requiring an MSYS2/MinGW shell.

  • Ccache is also now universally supported to improve build performance. Compile times should be further reduced with the addition of precompilation of certain headers.

  • A variety of other build system generators are now supported on all platforms, including multi-configuration Ninja and applicable types of Makefiles.

  • Compiler diagnostic flags with clang have been standardized across all platforms where possible1. Currently, diagnostic flags have rough parity with the old build system, but one goal of the new build system is to introduce more strict compiler settings, to eliminate certain classes of bugs and generally increase code quality.

Supported Platforms

ARM64 and x86_64 architectures are officially supported across macOS, Windows, Linux, and BSD1. clang, Clang-CL, gcc, and MSVC are all supported on applicable platforms. MSYS2/MinGW is supported along with configuration and builds via the default Windows shell.

32-bit builds have not been tested.

Current Status

Builds across all three of macOS, Windows, Linux, and BSD are generally expected to work. Detailed build instructions can be found on the new ares wiki. CI workflows are present and implemented but are still a work in progress. More complete documentation is also a work in progress.

macOS

  • Xcode, Ninja, Ninja Multi-Config, and Makefile generators are functional
  • Unlike Windows, multi-architecture builds are supported; pass -DCMAKE_OSX_ARCHITECTURES as appropriate.
  • Builds should generate a fully functional and relocatable app bundle within the build directory, with relative rpaths; install rules are placeholders.

Windows

  • Visual Studio 17 2022, Ninja, Ninja Multi-Config, and Makefile generators are functional
  • Only single-architecture builds are currently supported
  • Similarly to macOS, the build step generates a fully functional and relocatable rundir with placeholder install commands.

Linux / BSD

  • Unlike on macOS and Windows, it is required that the developer have packages installed for libraries ares links against. A list of required and optional libraries is found in the README.
  • Build will output to a staging directory rundir; ares only uses system libraries so this rundir should be fully relocatable
  • install will perform a standard installation as CMake deems appropriate for your distribution; you may pass the --prefix argument as needed
  • Maintainers may package as appropriate; a new Flatpak may be created as part of this process, but has not yet

Important

These tasks should be completed before this PR moves out of the draft state:

  • CI workflows - incomplete
  • BSD support
  • Windows SDL added to ares-deps
  • ares-deps should probably be migrated to the ares-emulator organization
  • More documentation
  • Miscellaneous cleanup, fixing issues found in testing, incorporating general feedback

Nevertheless, now that builds are functioning on all three platforms, I wanted to get this PR out there in a draft state to solicit testing and feedback, as well as to give Linux package maintainers a head start in terms of packaging.

Note

While the PR is in a draft state, there may be breaking changes, things may temporarily stop functioning, commits may be squashed, etc. Thank you for testing and please report any issues you encounter!

  • More detailed documentation on the internals of the build system is available here.

Miscellaneous changes/points

  • .gitignore is reorganized
  • gersemi is used for CMake formatting, hence the presence of a .gersemirc file
  • SDL on Windows is now a dynamic dependency.
  • macOS asset files are reworked into .xcassets archives
  • new macOS local build script introduced
  • CI will include debug symbols for easier crash debugging

Footnotes

  1. Indicates something that might not be finished yet, or is subject to change 2

@LukeUsher LukeUsher marked this pull request as ready for review November 1, 2024 07:21
@bsdcode
Copy link
Contributor

bsdcode commented Nov 1, 2024

Thanks for providing the draft. I'll happily work on bringing the FreeBSD support in the new build system up to date. I don't have experience with the other two major BSDs, OpenBSD and NetBSD, but at least for FreeBSD we can get 99% of the work done by just building like the Linux target. I'll report back soon.

I have already encountered a build failure trying to build your cmake branch:

/tmp/jcm93/build> cmake .. -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -G "Ninja Multi-Config"
CMake Error at CMakeLists.txt:7 (project):
  VERSION "1cae50b7a" format invalid.

That's because ARES_VERSION and ARES_VERSION_CANONICAL are directly derived from git describe --always --tags --dirty=-modified:

https://github.com/jcm93/ares/blob/1cae50b7a3b30f67a9c41acdeac7079765eae8da/cmake/common/versionconfig.cmake#L5-L22

After the regex replacement we get the commit hash 1cae50b7a for ARES_VERSION_CANONICAL, which then is used for the project version:

https://github.com/jcm93/ares/blob/1cae50b7a3b30f67a9c41acdeac7079765eae8da/CMakeLists.txt#L7

Probably document in README.md to set ARES_VERSION_OVERRIDE and ARES_VERSION_CANONICAL explicitly in the cmake invocation for development builds?

And using ARES_VERSION_OVERRIDE needs the following fix, or else it's using the literal value ARES_VERSION_OVERRIDE for ARES_VERSION:

bsdcode@e0e5d68

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 1, 2024

I have already encountered a build failure trying to build your cmake branch:

The code expects to live on the ares-emulator/ares repo, and the version derivation seeks out tags, which my fork doesn't have. I can add a better failure mode for this case, but in the meantime you can fix this by adding ares-emulator/ares as an upstream remote, then just fetching it to pull down tags.

@jcm93 jcm93 marked this pull request as draft November 1, 2024 17:12
@bsdcode
Copy link
Contributor

bsdcode commented Nov 3, 2024

but in the meantime you can fix this by adding ares-emulator/ares as an upstream remote, then just fetching it to pull down tags.

Thanks, yes this solves my issue.

I made some attempts to improve the FreeBSD support for the overhauled build system in my branch https://github.com/bsdcode/ares/tree/cmake, which is rebased on your cmake branch.

The commit bsdcode@0b34c49 mostly puts missing cmake/os-freebsd.cmake files into place, copied from cmake/os-linux.cmake in general, because the build process is the same between Linux and FreeBSD in large parts.

Commit bsdcode@3a3477d enables the ALSA audio driver by default again (an ON was missing in your last commit).

Unfortunately there's still a problem with the FreeBSD build:

[3/6] Linking CXX executable mia/mia
FAILED: mia/mia
: && /usr/bin/c++ -O2 -g -DNDEBUG  nall/CMakeFiles/nall.dir/main.cpp.o nall/CMakeFiles/nall.dir/nall.cpp.o nall/CMakeFiles/nall.dir/sljitAllocator.cpp.o mia/CMakeFiles/mia-ui.dir/mia.cpp.o mia/CMakeFiles/mia-ui.dir/resource/resource.cpp.o -o mia/mia  -Wl,-rpath,/usr/local/lib:  hiro/libhiro.a  ares/libares.a  thirdparty/tzxfile.a  /usr/local/lib/libgtk-3.so  /usr/local/lib/libgtk-3.so  /usr/local/lib/libgdk-3.so  /usr/lib/libz.so  /usr/local/lib/libpangocairo-1.0.so  /usr/local/lib/libpango-1.0.so  /usr/local/lib/libharfbuzz.so  /usr/local/lib/libatk-1.0.so  /usr/local/lib/libcairo-gobject.so  /usr/local/lib/libcairo.so  /usr/local/lib/libgdk_pixbuf-2.0.so  /usr/local/lib/libgio-2.0.so  /usr/local/lib/libgobject-2.0.so  /usr/local/lib/libglib-2.0.so  /usr/local/lib/libintl.so  /usr/local/lib/libX11.so  thirdparty/chdr-static.a  thirdparty/libchdr/deps/lzma-24.05/lzma.a  thirdparty/libchdr/deps/zlib-1.3.1/z.a  thirdparty/libchdr/deps/zstd-1.5.6/build/cmake/lib/zstd.a  -pthread  thirdparty/sljit.a  libco/liblibco.a  -lX11  -lXext && cd /tmp/ares/build/mia && /usr/local/bin/cmake -E make_directory /tmp/ares/build/rundir/RelWithDebInfo/. && /usr/local/bin/cmake -E copy_if_different /tmp/ares/build/mia/mia /tmp/ares/build/rundir/RelWithDebInfo/.
ld: error: unable to find library -lX11
ld: error: unable to find library -lXext
c++: error: linker command failed with exit code 1 (use -v to see invocation)
ninja: build stopped: subcommand failed.

The library path for some modules aren't propagated to the build system. In the case of -lX11 -lXext it was previously handled by explicitely setting -L/usr/local/lib in hiro/GNUmakefile. With cmake, it currently does a find_package(X11) which finds /usr/local/share/cmake/Modules/FindX11.cmake. I don't know if it's expected to feed the library path back to our build process (I'm definitly no cmake expert). The problem also occurs for -lpulse later in the build process (other libraries might also be affected, but I didn't examine it beyond this for now). I have /usr/local/lib/cmake/PulseAudio/PulseAudio.cmake on my system.

I do realize that this problem doesn't occur for modules for which you provide Finder Modules under cmake/finders, eg. the GTK module sets the library directory, which it gets from pkg-config. In essence that's what the old build system did, it used pkg-config to get the CFLAGS and LIBS for the dependencies.

I don't know what's the proper way to tackle the problem. Might additional finder modules help? For now I add link_directories("/usr/local/lib") to my private builds to overcome this problem. With that the FreeBSD build succeeds. But that's not the proper way to handle this of course.

I would expect the same problems in the Linux build, because I would be very surprised if the build behaviour differs in that regard between Linux and FreeBSD systems.

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 3, 2024

The library path for some modules aren't propagated to the build system. In the case of -lX11 -lXext it was previously handled by explicitely setting -L/usr/local/lib in hiro/GNUmakefile. With cmake, it currently does a find_package(X11) which finds /usr/local/share/cmake/Modules/FindX11.cmake. I don't know if it's expected to feed the library path back to our build process (I'm definitly no cmake expert). The problem also occurs for -lpulse later in the build process (other libraries might also be affected, but I didn't examine it beyond this for now). I have /usr/local/lib/cmake/PulseAudio/PulseAudio.cmake on my system.

Could you clear your build directory, then run generation with the options --debug-find --log-level debug, then send me the entire output?

Assuming you have the development package for X11 installed (if this exists on BSD), the module that CMake found there should be sufficient to propagate all the necessary information and libraries back up to our targets, and I prefer not to write find modules unless it is required to. But if it isn't for any reason working out of the box then we will indeed move to a find module.

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 3, 2024

And thanks very much for your work!

@bsdcode
Copy link
Contributor

bsdcode commented Nov 4, 2024

Could you clear your build directory, then run generation with the options --debug-find --log-level debug, then send me the entire output?

Of course. cmake .. -G Ninja --debug-find --log-level=debug > debug.txt 2>&1 produces debug.txt.

Assuming you have the development package for X11 installed (if this exists on BSD)

Packages aren't split into development and non-development packages on FreeBSD in general. So the relevant headers and libraries are installed. In fact, the build system does find the shared libraries in their correct places and propagates them, e.g. /usr/local/lib/libX11.so and /usr/local/lib/libXext.so. But the library path itself, here /usr/local/lib, isn't propagated and the linker then chokes on -lX11 -lXext.

And thanks very much for your work!

No problem, I'm glad to help.

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 4, 2024

I'm taking a look at this; I don't think that -lX11 and -lXext should actually be in there in the first place, since both libraries are already explicitly enumerated in the linker command. If we remove X11 from the target_link_libraries commands for both ruby and hiro, -lX11 -lXext are still there in the final linker command for some reason. Investigating...

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 4, 2024

I think it's something in the chdr-static target that is adding -lX11 to the linker flags for anything linking against it, but I don't see exactly where it's coming from yet. That area is something of a mess because it mixes our build code with various pieces of third party build code.

Cleaning that up should be something of a priority; ideally we will not have any -l-type linker search flags at the end of the build, since CMake's whole job is to locate these libraries explicitly for us and fail appropriately if they aren't found (which it's doing succesfully, we just seem to have pollution from a dependency currently).

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 4, 2024

Turns out the problem was me all along; the ares target was linking against bare "X11" and "Xext" here which is wrong; if those aren't located as CMake targets, then CMake will eventually just pass them with -l to the linker as a last ditch effort. This is one of the first things I wrote when stubbing out the Linux targets and I had totally forgotten about it. Oof!

@bsdcode
Copy link
Contributor

bsdcode commented Nov 5, 2024

Thanks for fixing this issue. I rebased my branch on your latest commits and applied a small fix to the PulseAudio find module (typo in PC_PulseAudio_CFLAGS_OTHER and empty PulseAudio_VERSION generated an error). Now the FreeBSD build succeeds.

I need to test the cmake build system a little bit more, but in general I'd say FreeBSD support is there again ;)

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 5, 2024

Cool, and thanks for the fixes there.

If you don't mind hanging tight, I still need to decide what I'm doing about the commit history, how many commits we want, etc. So I might make some more commits and squash and ask you to rebase, or I may just pull your changes in; I'll figure it out. In any case thanks again (and do let me know if you run into anything else in testing).

@bsdcode
Copy link
Contributor

bsdcode commented Nov 6, 2024

If you don't mind hanging tight, I still need to decide what I'm doing about the commit history, how many commits we want, etc. So I might make some more commits and squash and ask you to rebase, or I may just pull your changes in; I'll figure it out. In any case thanks again (and do let me know if you run into anything else in testing).

Yes no problem, I'll continue rebasing until you figure it out. Feel free to squash, apply or commit my changes in any way you like.

The configuration process had some warnings for the AO and librashader modules. So I cleaned the finder modules up a little bit. I also added the version info to the GTK finder module. This way we get some version infos for the dependencies in the configuration process, of course this is solely cosmetic. I squashed all changes to the finder modules into one commit, this includes the previous PulseAudio fix.

I added a check for ARES_SKIP_DEPS to the install targets for the slang shaders in desktop-ui/cmake/os-*.cmake. Otherwise if we don't download the shaders via ARES_SKIP_DEPS=ON (which will be the case for the FreeBSD port and packaging purposes, also Linux packagers might handle it that way), the install target would fail because the shaders aren't there.

I also realized that the udev and uhid input drivers, and the pulseaudio_simple audio driver, aren't included yet. Is this on purpose or should they be added back?

The VULKAN feature is currently enabled unconditionally in ares/CMakeLists.txt, regardless if the N64 core is build:

target_enable_feature(ares "N64 Vulkan rendering with paraLLEl-RDP" VULKAN)
target_compile_definitions(ares PUBLIC VULKAN)

I think this is only cosmetic, but should it be only set in ares/n64/CMakeLists.txt?

With the current librashader finder module the shared library librashader.so.2 is linked against the ares executable. I think this should not be neccessary, because librashader_ld.h dynamically loads the library at runtime. At least this was the behaviour under the previous build system. Should this be changed?

And last but not least, the build process generates the tools genius, mame2bml, sourcery, mia and i8080. My understanding is that these are development tools for the ares project. Should they be build unconditionally, or should they be behind somekind of ARES_ENABLES_TOOLS build flag?

@thekovic
Copy link
Contributor

thekovic commented Nov 6, 2024

I'm still not sure what i8080 is personally

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 6, 2024

I added a check for ARES_SKIP_DEPS to the install targets for the slang shaders in desktop-ui/cmake/os-*.cmake. Otherwise if we don't download the shaders via ARES_SKIP_DEPS=ON (which will be the case for the FreeBSD port and packaging purposes, also Linux packagers might handle it that way), the install target would fail because the shaders aren't there.

I'll probably handle this by abstracting the location for the shaders into a variable; if ARES_ENABLE_LIBRASHADER is on, failing to install shaders is a legitimate install failure (edit: after thinking about this, maybe it isn't and there should be a bit more abstraction); in any case, having hardcoded their location into the .deps path is not correct and I'll take a look at it.

I think that slang-shaders not having real distribution is an upstream issue, hence my willingness to try to pre-package them in ares-deps, but yeah, I'd like to leave it free for maintainers/packagers to handle however they see appropriate.

With the current librashader finder module the shared library librashader.so.2 is linked against the ares executable. I think this should not be neccessary, because librashader_ld.h dynamically loads the library at runtime. At least this was the behaviour under the previous build system. Should this be changed?

Kinda complicated; I would like to be able to know at build-time if librashader is going to be functional, and actually linking lets us verify we have the right ABI and architecture for the target platform, among other things; it makes the build more debuggable. I'll give it some thought though.

I also realized that the udev and uhid input drivers, and the pulseaudio_simple audio driver, aren't included yet. Is this on purpose or should they be added back?

Not intentional, just slipped through; I'll re-add them (and figure out what pulse simple is vs. ordinary Pulse).

And last but not least, the build process generates the tools genius, mame2bml, sourcery, mia and i8080. My understanding is that these are development tools for the ares project. Should they be build unconditionally, or should they be behind somekind of ARES_ENABLES_TOOLS build flag?

I think it's fine to have them built when building all targets, since they are indeed currently part of the project. --target desktop-ui can always be specified for only building ares itself.

@bsdcode
Copy link
Contributor

bsdcode commented Nov 6, 2024

I'll probably handle this by abstracting the location for the shaders into a variable; if ARES_ENABLE_LIBRASHADER is on, failing to install shaders is a legitimate install failure (edit: after thinking about this, maybe it isn't and there should be a bit more abstraction); in any case, having hardcoded their location into the .deps path is not correct and I'll take a look at it.

I think that slang-shaders not having real distribution is an upstream issue, hence my willingness to try to pre-package them in ares-deps, but yeah, I'd like to leave it free for maintainers/packagers to handle however they see appropriate.

Ah ok, I understand your thought. I looked at some Linux packaging for ares (Debian, OpenSuse, Arch and Flatpak). They all seem to install the vendored slang shaders from ares.

On FreeBSD we have a seperate port and package for the slang shaders from the libretro project. Because we don't want to duplicate the shaders, the ares port doesn't package the vendored shaders from ares, but instead I apply a small patch in order to look for the location of the shaders from the libretro package as a last-resort fallback if ares can't find shaders in its own datadir.

With this usecase in mind it's preferable to have librashader support with ARES_ENABLE_LIBRASHADER and also allow to not install the shaders via ARES_SKIP_DEPS=ON.

I think it's fine to have them built when building all targets, since they are indeed currently part of the project. --target desktop-ui can always be specified for only building ares itself.

That's fine.
There's a problem with building --target desktop-ui: it still builds sourcery and cmake --install . fails with

-- Install configuration: "RelWithDebInfo"
CMake Error at mia/cmake_install.cmake:47 (file):
  file INSTALL cannot find "/tmp/ares/build/mia/mia": No such file or
  directory.
Call Stack (most recent call first):
  cmake_install.cmake:77 (include)

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 6, 2024

There's a problem with building --target desktop-ui: it still builds sourcery and cmake --install . fails

desktop-ui building sourcery is correct because sourcery creates resource.cpp files that are compiled as part of the desktop-ui build.

There isn't an out-of-the-box way to install only certain targets in CMake, so that is failing just because the mia executable isn't built.

The idiomatic solution for only installing some things is to define install "components". I think this might be easy but I haven't looked at it yet. I will though.

On FreeBSD we have a seperate port and package for the slang shaders from the libretro project

In this case I might take a look at treating slang-shaders as a proper CMake target and searching for it like other libraries, because I'd like to make life easier for anyone packaging/distributing things properly. I don't know how weird this is for libraries that are "data-only"; and it's also possible it might require some upstreaming work, since it's a bit awkward if I'm looking for slang-shaders in a way that's specific only to BSD. I'll look at how that package is defined compared to the upstream (and the relevant CMake stuff) and report back.

@bsdcode
Copy link
Contributor

bsdcode commented Nov 6, 2024

since it's a bit awkward if I'm looking for slang-shaders in a way that's specific only to BSD. I'll look at how that package is defined compared to the upstream

There's nothing special to it. The port just makes a package out of the contents of the slang-shaders repository. The FreeBSD port just installs the shaders into usr/local/share/libretro/shaders/shaders_slang. Arch has also such a package and installs them into usr/share/libretro/shaders/shaders_slang.

In this case I might take a look at treating slang-shaders as a proper CMake target and searching for it like other libraries, because I'd like to make life easier for anyone packaging/distributing things properly. I don't know how weird this is for libraries that are "data-only";

I think such a cmake finder module could be similar to your FindOSS.cmake module. Just try to find a sensible shader file, like libretro/shaders/shaders_slang/bilinear.slangp, in the PATHS /usr/share and /usr/local/share.

And thanks for trying to make life easier for us packagers :)

@Screwtapello
Copy link
Contributor

and figure out what pulse simple is vs. ordinary Pulse

libpulse-simple.so is just a wrapper around libpulse.so. The PulseAudio API is very big and complex and intimidating if you just want to generate a simple fixed-sample-rate audio-stream, so the pulse-simple API makes it easy to do that as, as long as you don't also need the resampling and multi-stream mixing and video synchronisation and such that the full API provides.

You'd probably expect that it would be enough to have a driver for libpulse or libpulse-simple and not both, but empirically I have found that one worked beautifully and the other had crackling, while other people reported the other way around. So, Near left them both in.

@LukeUsher
Copy link
Member

I'm still not sure what i8080 is personally

A test hardness for Intel 8080 CPU emulation

@thekovic
Copy link
Contributor

thekovic commented Nov 7, 2024

Ah, it's in the tests folder. How did I miss that? Anyway, I think in that case it shouldn't be getting compiled if I specify select cores (for example DARES_CORES="n64". However, reading the replies that have come since my first comment, I think that has been solved now?

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 7, 2024

A few changes:

  • install components are defined; not sure if they are fully working as intended yet
  • slang-shaders is now a proper target with a find module, and the option to bundle slang-shaders with the install is behind a new Linux-specific option ARES_BUNDLE_SHADERS
    • corresponding changes in ares-deps are here
    • Logically, shaders will now only be installed if librashader is enabled, slang-shaders are found, and ARES_BUNDLE_SHADERS is true
  • This will need to be spun out into a separate PR, but the nall path.cpp semantics were not correct; changes here are not final

@bsdcode
Copy link
Contributor

bsdcode commented Nov 8, 2024

Rebased my branch. Additional fixes:

  • Only set the librashader target_link_library if librashader_FOUND is true, otherwise the build process fails if librashader is not installed (either by package manager or by ares.deps).
  • Strip whitespaces from the elements of ARES_CORES, because some porting environments might add a whitespace to the elements when constructing the ARES_CORES list, e.g. ARES_CORES="md; n64;". Then the test if(n64 IN_LIST ARES_CORES) always fails and the n64 core isn't build.

Thanks for the slang-shaders find module. Do you think we can use the slang_shaders_LOCATION as an additional search path for the shaders (in a separate PR)? With this I could get rid of my small patch which searches for shaders in /usr/local/share/libretro/shaders/shaders_slang/ as a fallback.

I'm already working on a work-in-progress update of the FreeBSD ares port which uses cmake. It already simplifies and streamlines the port quite a bit and works really well. So thanks :)

@jcm93
Copy link
Contributor Author

jcm93 commented Nov 8, 2024

Only set the librashader target_link_library if librashader_FOUND is true, otherwise the build process fails if librashader is not installed (either by package manager or by ares.deps).

librashader always being linked is on purpose, and you should not override this behavior. if librashader_FOUND is false, then linking against librashader means linking against the INTERFACE version of the target, which does not define a library and only defines a header include directory (the header being bundled with ares-deps; the library itself is NOT included with ares-deps on Linux). The header is strictly necessary to successfully compile ares (the build fails if it cannot be found), hence the unconditional linkage.

Do you think we can use the slang_shaders_LOCATION as an additional search path for the shaders (in a separate PR)? With this I could get rid of my small patch which searches for shaders in /usr/local/share/libretro/shaders/shaders_slang/ as a fallback.

Yes; the intention of the find module and #1688 was to enable this future work. Note that I would not attempt to pass that actual variable into ares, but rather just tell ares to look for shaders in standard paths once 1688 lands with the libretro/shaders/shaders_slang/<the>/<shader>.slangp relative path appended.

jcm93 and others added 10 commits December 25, 2024 18:46
Co-authored-by: Stefan Schlosser <[email protected]>
Co-authored-by: Stefan Schlosser <[email protected]>
Fixes up the release action to produce nightly builds under the "nightly" tag for any commits on master, duplicating the legacy release pattern.
* Use RelWithDebInfo as the launch configuration in the default scheme

* Assume IPO is supported under Xcode, since checking takes over 10 seconds for some reason
@jcm93
Copy link
Contributor Author

jcm93 commented Dec 26, 2024

Performed one more large interactive rebase to make formatting consistent; CMake code in all commits is now formatted by gersemi; further, scripts/cmake-format.sh has been added to format all of the project's CMake code in place.

@Mastergatto
Copy link
Contributor

Hi, many thanks for the response! I have noticed that even with -DARES_BUNDLE_SHADERS=OFF -DARES_SKIP_DEPS=ON, it still copies shader files in build/rundir/share/ares/Shaders (ultimately they don't end up in the package, though).
I would like to avoid even the copy part, to not waste space on disk for everyone.

@Mastergatto
Copy link
Contributor

One more question and I believe the PKGBUILD I'm updating will be ready for the moment this PR is merged. How do I turn off GTK's 52 (!) deprecation warnings? I tried to pass -Wno-deprecated-declarations to CMake, but to no avail.

@jcm93
Copy link
Contributor Author

jcm93 commented Dec 27, 2024

It's intentional that diagnostic flags including -Wdeprecated-declarations are set by the project and not (trivially) overrideable. This is to call increased attention to various aging components and other code issues in ares as the project moves forward. This is a break with past practices, but I believe it is a good idea as ares ages and updates become increasingly necessary.

In hiro, deprecation warnings on Cocoa are remaining, because issues there should be fixed. I am admittedly not very well versed in Linux development, and I haven't looked at how important it is that the warnings GTK currently throws be fixed. If they are deemed insignificant, or otherwise not fixable because of hiro's design, the appropriate course will be to disable them on a more granular level, either in code or target-wide in hiro's Linux CMake code.

If the warnings are a blocker for you, let me know, but otherwise I'd prefer to kick the can down the line until appropriate attention can be given to them.

@Mastergatto
Copy link
Contributor

Mastergatto commented Dec 27, 2024

I understand, though in the case of GTK, IMHO these deprecation warnings can be safely disregarded, given that GTK3 is outdated and currently it is in maintenance mode: this means that it will receive no new features or any other API changes, and even then GTK3 is here to stay for a long time, as it happened with GTK2.

As for these warnings, in the GTK world, these are useful to developers that want to port the GUI to GTK4. Not to users that build from source, and not to maintainers like me: they may see these warnings, but they don't know what to do with them. I'd rather not let users see them, but in the end they aren't a blocker for me, though.

@jcm93
Copy link
Contributor Author

jcm93 commented Dec 27, 2024

I'll take another look when I have time; I hadn't really thought about the PKGBUILD case. In fact, thinking about it more, I'm not 100% sure that no-deprecated-declarations passed at configure time shouldn't override the deprecated-declarations I set; will have to refresh myself on how CMake ends up handling conflicting flags.

@jcm93
Copy link
Contributor Author

jcm93 commented Dec 29, 2024

So on a technical level, things are fairly simple; the last flag added in the compile invocation ends up with precedence. add_compile_options flags from the project are added after CXX_FLAGS passed via the command line, so it's unlikely that overriding them will work if two are in direct conflict.

Meanwhile, I do agree with you that for non-developer users building from source, deprecation warnings are essentially noise.

However, letting in a variable to control whether these warnings appear is a slippery slope. Overall, that would not be in line with the goal of enforcing compiler warnings that developers of ares should care about, regardless of platform, in order to fix various problems and code smells, and overall prioritize keeping ares in better shape as we move into the future.

(Aside: the deprecation warnings being relevant for porting to GTK4 is definitely valuable, because we will want to port hiro to GTK4, plus remove our hard dependency on X11, among other concerns, as we move forward).

So where I'm landing for now is: If you want to get rid of the -Wdeprecated-declarations for non-dev users building via your AUR package, I would recommend applying a patch to cmake/common/compiler_common.cmake (or cmake/linux/compilerconfig.cmake) to remove the warning. I'd also note that I likely will try to increase compiler strictness in the future as issues are fixed, so that will be an evolving area of the build. Otherwise, we should work on the warnings that are relevant for moving toward GTK4 and leave them in, or, if they're irrelevant, suppress them in a separate work item.

@Mastergatto
Copy link
Contributor

That's fair. I'll have to consider whether to do the patch you just suggested, but I don't think it's worth it for some output lines which has no impact anyway.

Is there an estimate on when this PR will be merged?

Thanks again for your continued support!

P.S.: looks like there's willingness to do the GTK4 port in future if I'm not mistaken, that would be a good thing to do because I think for sure Ares needs a more modern interface that works well, that doesn't require X11/Xlib anymore and allows things like fractional scaling to work properly.

@jcm93
Copy link
Contributor Author

jcm93 commented Dec 29, 2024

I think it is ready now barring any additional feedback.

The only final chores remaining are:

  • The org version of ares-deps has a typo in the release tag, and this branch still points at the ares-deps repo under my user. Once we have a release cut with a correct tag I can point CMake at the org version.

Once this branch is merged:

  • We need to remove the ARES_VERSION_OVERRIDEs present in the CMakePresets.json file
  • Enable the new CI workflow and either disable the old workflow.

@LukeUsher LukeUsher merged commit 8433666 into ares-emulator:master Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants