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.zig: Add ability to install a shared library #3680

Closed
wants to merge 1 commit into from

Conversation

greenfork
Copy link
Contributor

@greenfork greenfork commented Dec 27, 2023

Initial build.zig only installed a static library. This is fine for
Zig itself but not sufficient for other ecosystems.

Current patch doesn't break backwards compatibility but does a little
refactoring where the old function addRaylib uses the newly added
function addRaylibTo, which is more flexible. In fact, addRaylib is an
alias for addStaticRaylib. addRaylib still generates a static library,
whereas the newly added addSharedRaylib generates a shared library.

Additionally, since a shared library is also a common artifact,
build.zig also installs it to a default location of
zig-out/lib/libraylib.dll.

Example code how to generate a shared library artifact in Zig:

const libraylib = raylib.addSharedRaylib(b, target, optimize, .{});

// Define a USE_LIBTYPE_SHARED macro for your project that is used
// in raylib.h when using a shared library. Your project must use
// raylib.h in order for this macro to work. It is not necessary
// but saves performance when using on Windows. For more see:
// https://github.com/raysan5/raylib/pull/3680#issuecomment-1870644148
my_project.defineCMacro("USE_LIBTYPE_SHARED", null);

@Peter0x44
Copy link
Contributor

Shouldn't this be defining BUILD_LIBTYPE_SHARED? or is that done elsewhere?

@greenfork
Copy link
Contributor Author

I didn't know about this constant, let me rework this a bit. Thanks!

@greenfork greenfork marked this pull request as draft December 27, 2023 21:35
@Peter0x44
Copy link
Contributor

Peter0x44 commented Dec 27, 2023

I didn't know about this constant, let me rework this a bit. Thanks!

It's required to build a usable DLL.
Also, anything consuming the shared lib should be defining USE_LIBTYPE_SHARED, though usually you can get away without it. I think the function calls have to go through some sort of "trampoline" instead and are less efficient though.

greenfork added a commit to greenfork/raylib that referenced this pull request Jan 3, 2024
Initial build.zig only installed a static library. This is fine for
Zig itself but not sufficient for other ecosystems.

Current patch doesn't break backwards compatibility but does a little
refactoring where the old function addRaylib uses the newly added
function addRaylibTo, which is more flexible. In fact, addRaylib is an
alias for addStaticRaylib. addRaylib still generates a static library,
whereas the newly added addSharedRaylib generates a shared library.

Additionally, since a shared library is also a common artifact,
build.zig also installs it to a default location of
zig-out/lib/libraylib.dll.

Example code how to generate a shared library artifact in Zig:

    const libraylib = raylib.addSharedRaylib(b, target, optimize, .{});

    // Define a USE_LIBTYPE_SHARED macro for your project that is used
    // in raylib.h when using a shared library. Your project must use
    // raylib.h in order for this macro to work. It is not necessary
    // but saves performance when using on Windows. For more see:
    // raysan5#3680 (comment)
    my_project.defineCMacro("USE_LIBTYPE_SHARED", null);
Initial build.zig only installed a static library. This is fine for
Zig itself but not sufficient for other ecosystems.

Current patch doesn't break backwards compatibility but does a little
refactoring where the old function addRaylib uses the newly added
function addRaylibTo, which is more flexible. In fact, addRaylib is an
alias for addStaticRaylib. addRaylib still generates a static library,
whereas the newly added addSharedRaylib generates a shared library.

Additionally, since a shared library is also a common artifact,
build.zig also installs it to a default location of
zig-out/lib/libraylib.dll.

Example code how to generate a shared library artifact in Zig:

    const libraylib = raylib.addSharedRaylib(b, target, optimize, .{});

    // Define a USE_LIBTYPE_SHARED macro for your project that is used
    // in raylib.h when using a shared library. Your project must use
    // raylib.h in order for this macro to work. It is not necessary
    // but saves performance when using on Windows. For more see:
    // raysan5#3680 (comment)
    my_project.defineCMacro("USE_LIBTYPE_SHARED", null);
@greenfork greenfork marked this pull request as ready for review January 3, 2024 14:26
@raysan5
Copy link
Owner

raysan5 commented Jan 3, 2024

@greenfork It seems this is a considerable redesign of the build.zig, adding an extra level of complexity and maintenance cost. Is it really required? Would this change improve the experience for most of the users requiring this script?

I'm not using neither maintaining this script but I think other users should review this PR.

@greenfork
Copy link
Contributor Author

@Peter0x44 I have addressed your comment and also ported -fvisibility=hidden from the Makefile:

raylib/src/Makefile

Lines 386 to 397 in e46b614

# Define required compilation flags for raylib SHARED lib
ifeq ($(RAYLIB_LIBTYPE),SHARED)
# make sure code is compiled as position independent
# BE CAREFUL: It seems that for gcc -fpic is not the same as -fPIC
# MinGW32 just doesn't need -fPIC, it shows warnings
CFLAGS += -fPIC -DBUILD_LIBTYPE_SHARED
# hide all symbols by default, so RLAPI can expose them
ifeq ($(PLATFORM_OS),$(filter $(PLATFORM_OS), LINUX BSD OSX))
CFLAGS += -fvisibility=hidden
endif
endif

In order to port -fvisibility=hidden I also had to change from static flag concatenation to dynamically allocated array or static strings.

@greenfork
Copy link
Contributor Author

@greenfork It seems this is a considerable redesign of the build.zig, adding an extra level of complexity and maintenance cost. Is it really required? Would this change improve the experience for most of the users requiring this script?

I'm not using neither maintaining this script but I think other users should review this PR.

It does change a lot of lines, but conceptually it is not so scary. The patch essentially does these things:

  1. addRaylib is split into two functions addRaylib and addRaylibTo.
  2. addSharedRaylib function is added that uses addRaylibTo.
  3. raylib_flags now use allocated array instead of a static array.

The result is that Zig can now build a shared library and, more importantly, cross-compile it to different systems. Users of Raylib can now develop on Linux and cross-compile a dynamic library for Windows. For me this is a very essential functionality because my language of choice can only use shared C libraries and I would like to do cross-compilation of my games from Linux to Windows.

PS: The cross-build for MacOS is broken at the moment both on master and on this branch, this will require some input from MacOS users.

@greenfork
Copy link
Contributor Author

I'm not using neither maintaining this script but I think other users should review this PR.

@raysan5 should I find someone or there's someone already and I should just wait?

@greenfork
Copy link
Contributor Author

Also, I have a question about the goal of build.zig. Does it strive to be a fully featured build system like existing Make and CMake or is this just a tool for some specific purpose? @raysan5 when you say that added complexity may be not worth it, does it mean that you don't consider adding a shared library generation by Zig or that you would like to achieve it with a lesser cost?

@raysan5
Copy link
Owner

raysan5 commented Jan 4, 2024

@greenfork The goal of build.zig was providing a simple alternative to default Makefile. By default raylib is compiled as a static library and that's the recommended usage... but I'm aware that at this point there are many raylib bindings that require a shared library.

I will ask in raylib Discord for any Zig experts that can review and evaluate this change.

@purrie
Copy link

purrie commented Jan 6, 2024

Line 5: To make a proper alias, you can just do

pub const addRaylib = addStaticRaylib;

Lines 25 and 46: Should use catch instead of if statement for error handling

Line 76: Not sure if this flag is needed, if you throw it away, you can throw away all the arena allocation since all other flags are comptime and you can merge them with ++ like it was done originally. Or could roll it into common flags since I don't think it does anything when building a static library. Then you can also get rid of the error handling altogether in the wrapping functions.

Lines 267+: I don't think you need to install headers for each library type since the headers are the same. Can get rid of the whole loop and just replace it with one extra installArtifact instead.

@purrie
Copy link

purrie commented Jan 6, 2024

As far as I'm concerned. I think adding a basic build script for Zig that can create a shared library is alright but I don't think it should go beyond just basic functionality. I use the provided build script for quick and dirty, get things setup fast and then migrate to writing my own customized build scripts as my projects get out of prototype state.

Since the code is already written, and could be cleaned up with a few steps then it could be fine to accept it but as a Zig user, I wouldn't need this functionality myself. Zig can very well use the library produced by make script with no issues, and fewer things to maintain is better.

@greenfork
Copy link
Contributor Author

Line 76: Not sure if this flag is needed, if you throw it away, you can throw away all the arena allocation since all other flags are comptime and you can merge them with ++ like it was done originally. Or could roll it into common flags since I don't think it does anything when building a static library. Then you can also get rid of the error handling altogether in the wrapping functions.

I have read about the -fvisibility=hidden flag and I'm not sure how it will interact with Zig, needs experimentation. GCC says that it supports it on Windows too and for clang I didn't find any information. I honestly don't know if this flag will work as expected on Windows when used from Zig.

Zig can very well use the library produced by make script with no issues

My main reason for this was easy cross-compilation. Zig allows for that with ease.


Unfortunately I failed to create bindings for Pony because of some strange interaction between the Pony runtime and Raylib. This patch requires more work to do and I will leave it for the next volunteer, closing this one.

@greenfork greenfork closed this Jan 7, 2024
@greenfork greenfork deleted the add-zig-shared-lib branch January 7, 2024 05:51
@Peter0x44
Copy link
Contributor

Peter0x44 commented Jan 7, 2024

My understanding of -fvisibility=hidden is that it doesn't affect windows at all
With windows, only the functions marked __declspec(dllexport) can be called.
An so by default exports everything. -fvisibility=hidden effectively gives you the windows behavior, except you need to use __attribute__((visibility("default"))).

GCC has a bit of special handling for dlls that lets you access them without the __declspec stuff, but you need to do it if visual studio compatibility matters.

I believe macos dylibs work the same way, but I'm not 100% on that, and I can't test it.

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.

4 participants