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 CMake build support #19

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

FtZPetruska
Copy link

Thie PR adds support for building with CMake (See #18). It is based on the existing scons script for files and flags used.

The CMake script includes:

  • Generation of CMake package files so the library can be used in other CMake projects by using find_package(libgambatte)
  • Generation of a pkg-config file, so the library can be used in non-CMake projects by using pkg-config --cflags --libs libgambatte
  • Building both the static and shared libraries at the same time.
  • Support for the testrunner.

There were a few code changes, mainly aiming at making building and installing more convenient:

  • Move newstate.h to the public headers, since gambatte.h depends on it, it should be public too
  • Instead of passing -DHAVE_X, prefer generating a config.h. This is necessary as gbint.h needs to know which of cstdint or stdint.h are available, but that information would be lost for a consumer of the library.
  • Edit the scons script to generate its own config.h, so the shell scripts keep working for the time being

The CI script has been completely reworked:

  • Run on Ubuntu, macOS, Msys2 (ucrt-x86_64, i686), Windows (clang-cl, clang)
  • Run the test runner
  • Test the CMake package (from an install and from the build directory)
  • Test the pkg-config file

Lastly, the README.md and INSTALL.md have been updated to remove the references to Qt, use CMake, and cover more common Linux distributions. Fixes #5


A few additional notes:

  • the macOS CI environment struggles to get the correct libpng (even specifying its prefix manually), so instead PNG_LIBRARY and PNG_PNG_INCLUDE_DIR are set manually to help the Find module. Causes the tests to SEGFAULT otherwise.
  • MSVC currently does NOT work, there is a VLA in src/statesaver.cpp:553 that breaks compilation. It is probably not a difficult fix, but this PR is already big enough as-is, and there may be more issues.

Since it is included in gambatte.h it needs to be installed.
The current implementation provides:
- Options to build both static and shared library at the same time
- A toggle for enabling warnings as errors
- Support for enabling zlib
- Generating pkg-config and CMake targets
- Exporting CMake targets to consume from the build directory
This is required as public header rely on knowing the value of
HAVE_STDINT_H and HAVE_CSTDINT.

This was also backported to scons since it edits code.
Changes include:
- building on multiple platforms
- run ctest tests
- test consuming from the build directory, from an installation, and
from pkg-config.
INSTALL.md Outdated Show resolved Hide resolved
INSTALL.md Outdated Show resolved Hide resolved
"libgambatte/src/mem/mbc/mmm01.cpp"
"libgambatte/src/mem/mbc/pocket_camera.cpp"
"libgambatte/src/mem/mbc/wisdom_tree.cpp"
"libgambatte/src/mem/snes_spc/dsp.cpp"
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should consider making snes_spc a separate "library" in this regard instead of bundled into libgambatte (not really any different in practice since it'd always just a static library included in libgambatte). Although that's probably suited for a later time.

- Classes and functions in public headers should be exported.
- Internal symbols should hidden.
- The "cinterface.cpp" module was left unmodified for compatibility.
- This reduces binary size for non-Windows platforms.
- For Windows, it allows using a DLL with the public API.
This allows using the same target when adding the project as a
subproject using `add_subdirectory`.
@FtZPetruska
Copy link
Author

Regarding symbols visibility, as it stood, the public API (gambatte.h) was not available when linking against the DLL.

On the other hand, the cinterface.cpp file explicitly exported functions despite them not be declared in any header.

To address that, I made the following changes in 4ea626a:

  • Made a public gbexport.h header that defines a GBEXPORT macro similar to what was defined in cinterface.h, with the GCC/Clang equivalent.
  • Exported every class/function declared in the public headers
  • Switch the Linux/macOS behaviour to match what Windows does (symbols hidden by default), this has the side-effect of slightly reducing the binary size too.
  • The cinterface.cpp file was left "as-is" for backwards compatibility. Since the GBEXPORT macro no longer contains the extern "C" part, it was added explicitly around these functions.

This change makes the shared library usable on Windows as previously programs such as the testrunner would fail to link dynamically.

@CasualPokePlayer
Copy link
Member

CasualPokePlayer commented Jun 4, 2023

Is this GBEXPORT on classes behavior for if you're using a shared library yet you're using the C++ API? You shouldn't be using that API directly in such a case, as that usually will end up resulting in linking errors and trying to dynamically link with dlopen/LoadLibrary would not exactly be fun to do (both due to compiler specific name mangling and potentially even other compiler specific ABI differences). You should only be using the C++ API if you're using the library as a static library (and you are in fact a C++ user and using the same compiler here).

To properly address the issue of a lack of a header, function declarations should just be added to cinterface.h (or whatever it's named now) for the cinterface.cpp functions. Programs such as the testrunner shouldn't be dynamically linking anyways (those statically link unless something changed in this PR), so something is going wrong much earlier there.

@FtZPetruska
Copy link
Author

Just to make sure I get this correctly:

  • We do not want the C++ API to be exported (gambatte::GB/NewState*/PakInfo classes), these constructs should only be usable from a static library.
  • When using the shared library, only the functions defined in cinterface.cpp should be accessible.

To properly address the issue of a lack of a header, function declarations should just be added to cinterface.h (or whatever it's named now) for the cinterface.cpp functions.

I am thinking about making it a gambatte-c.h header, and add the prototypes for the functions defines currently in cinterface.cpp. I think it may desirable to also build these functions in the static library, so they are always available regardless of the library type (in scons, cinterface.cpp was only included in shared builds).

Programs such as the testrunner shouldn't be dynamically linking anyways

I had made the static library a requirement to run the test:

if(NOT LIBGAMBATTE_BUILD_STATIC)
  message(FATAL_ERROR "Building the static library is required to run tests")
endif()

But it's good to know that you are not supposed to dynamically link it.

#define GBEXPORT
#endif

extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

Since this might be expected to be #included from a C context (not C++), it may be better to #ifdef this around __cplusplus.

Similarly, instead of #including gambatte.h or newstate.h, it may be better to just forward declare struct GB instead, (and other redeclarations from the gambatte namespace as that might not be available in a C context), at the very least do that when __cplusplus is not defined.

#include "newstate.h"

#if defined(_WIN32) || defined(__CYGWIN__)
#define GBEXPORT __declspec(dllexport)
Copy link
Member

@CasualPokePlayer CasualPokePlayer Jun 4, 2023

Choose a reason for hiding this comment

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

If this is used by the user of a dll rather than the dll itself, then dllexport would be incorrect, dllimport is the correct declspec (and I don't think visibility attribrute is correct either for non-Windows cases, rather it should just be extern)

Copy link
Author

Choose a reason for hiding this comment

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

Checking GCC's documentation on visibility, you're correct about the dllexport/dllimport.

According to that same documentation, for the non-Windows case, __attribute__((visibility("default"))) seems to indicate both export and import.

Copy link
Author

Choose a reason for hiding this comment

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

Seems like the change in d1ae9ec broke CI.

Looking around, other projects such as SDL seem to only be using dllexport.

Copy link
Member

Choose a reason for hiding this comment

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

Breaking CI likely indicates it got used internally in the library, or it got used when linking it in statically. You can only use dllimport when you are a user of the library (so not the library itself) and the library is a dll (if it's static, you don't use dllimport/dllexport).

dllexport is only meant to be used when building the library. When using
the API, dllimport should be used.

#ifdef __cplusplus
namespace gambatte {
extern "C" {
Copy link
Member

Choose a reason for hiding this comment

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

extern "C" here is a bit interesting, technically this is correct, although in practice it typically doesn't actually matter, as most (if not all practical) C++ implementations wouldn't have any issues with a C++ linkage function being assigned to a C linkage function pointer and vice versa (as they function the same at runtime), although this isn't actually guaranteed by the standard (C++ linkage could entail a different calling convention compared to the same function being assigned C linkage, in practice this never occurs).

On that, technically this is not correctly handled in gambatte.h as they all would use C++ (default) linkage, so if we want that to be ""correctly"" done, all those callback definitions need extern "C".

Copy link
Author

Choose a reason for hiding this comment

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

On the off chance that there is a compiler out there that defaults to using a different calling convention between C and C++, I went ahead and declared the other function prototypes as extern "C" for consistency.

Moreover, to make sure the C API is actually usable from C, I added a simple test in c75fe83 that creates and deletes a GB object using the C API.

This ensures the header is usable from a C source.
This is mainly for correctness, but ensures compatibility when __stdcall
is the default.
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.

README needs to be updated to account for core submodule change
2 participants