google / prefab

Prefab is a tool for generating build system integrations for prebuilt C/C++ libraries.
https://google.github.io/prefab/
Apache License 2.0
210 stars 32 forks source link

[FR] export_preprocessor_defs and export_includes #164

Open madebr opened 1 year ago

madebr commented 1 year ago

Is your feature request related to a problem? Please describe.

I would like to define extra CMake variables + modify dependencies in the XXXConfig.cmake files, generated by prefab.

Describe the solution you'd like Allow inclusion of random code in the form of inclusion of a cmake module script. This requires a modification of the prefab json scheme to list cmake modules.

Describe alternatives you've considered Extending the json scheme to list all properties/variables/... is a non-starter because nothing beats code.

Additional context

This missing feature comes up when wanting to make the SDL prefab package behave the same as ordinary SDL desktop packages.

For example:

A generated prefab SDL3Config.cmake file looks as follows:

if(NOT TARGET SDL3::SDL3)
add_library(SDL3::SDL3 SHARED IMPORTED)
set_target_properties(SDL3::SDL3 PROPERTIES
    IMPORTED_LOCATION "/home/maarten/.gradle/caches/transforms-3/289e2cc405c0bca25534e22271176741/transformed/SDL3-3.0.0/prefab/modules/SDL3/libs/android.x86/libSDL3.so"
    INTERFACE_INCLUDE_DIRECTORIES "/home/maarten/.gradle/caches/transforms-3/289e2cc405c0bca25534e22271176741/transformed/SDL3-3.0.0/prefab/modules/SDL3/include"
    INTERFACE_LINK_LIBRARIES ""
)
endif()

if(NOT TARGET SDL3::SDL3-static)
add_library(SDL3::SDL3-static STATIC IMPORTED)
set_target_properties(SDL3::SDL3-static PROPERTIES
    IMPORTED_LOCATION "/home/maarten/.gradle/caches/transforms-3/289e2cc405c0bca25534e22271176741/transformed/SDL3-3.0.0/prefab/modules/SDL3-static/libs/android.x86/libSDL3.a"
    INTERFACE_INCLUDE_DIRECTORIES "/home/maarten/.gradle/caches/transforms-3/289e2cc405c0bca25534e22271176741/transformed/SDL3-3.0.0/prefab/modules/SDL3-static/include"
    INTERFACE_LINK_LIBRARIES "-ldl;-lGLESv1_CM;-lGLESv2;-llog;-landroid;-lOpenSLES"
)
endif()

if(NOT TARGET SDL3::SDL3test)
add_library(SDL3::SDL3test STATIC IMPORTED)
set_target_properties(SDL3::SDL3test PROPERTIES
    IMPORTED_LOCATION "/home/maarten/.gradle/caches/transforms-3/289e2cc405c0bca25534e22271176741/transformed/SDL3-3.0.0/prefab/modules/SDL3test/libs/android.x86/libSDL3_test.a"
    INTERFACE_LINK_LIBRARIES ""
)
endif()

It would be nice if it would become possible to inject a cmake module (at the bottom of the generated SDL3Config.cmake) that modifies these a bit:

# Define CMake variables that SDL CMake users expect to exist
get_property(SDL3_INCLUDE_DIR TARGET SDL3::SDL3 PROPERTY INTERFACE_INCLUDE_DIRECTORIES)
set(SDL3_INCLUDE_DIRS "${SDL3_INCDIR};${SDL3_INCDIR}/SDL3")
set(SDL3_LIBRARIES SDL3::SDL3)
set(SDL3_STATIC_LIBRARIES SDL3::SDL3-static)
set(SDL3_STATIC_PRIVATE_LIBS)
set(SDL3TEST_LIBRARY SDL3::SDL3test)

# add "include/SDL3" to targets
set_property(TARGET SDL3::SDL3 APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${SDL3_INCLUDE_DIR}/SDL3")
set_property(TARGET SDL3::SDL3-static APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${SDL3_INCLUDE_DIR}/SDL3")
set_property(TARGET SDL3::SDL3test APPEND PROPERTY INTERFACE_INCLUDE_DIRECTORIES "${SDL3_INCLUDE_DIR}/SDL3")

# set compatible interface properties to avoid mixing shared and static SDL libraries
set_target_properties(SDL3::SDL3 PROPERTIES
    COMPATIBLE_INTERFACE_BOOL "SDL3_SHARED"
    INTERFACE_SDL3_SHARED "TRUE"
)
set_target_properties(SDL3::SDL3-static PROPERTIES
    COMPATIBLE_INTERFACE_BOOL "SDL3_SHARED"
    INTERFACE_SDL3_SHARED "FALSE"
)

This also allows packages to provide cmake functions etc.

DanAlbert commented 1 year ago

How would you propose to make this portable to other build systems? If CMake were the only backend there would be no need for prefab.

madebr commented 1 year ago

For includes/cflags, you can easily make it portable across build systems: add a export_includes/export_cflags field to module.json. e.g.

{
  "export_includes": ["", "SDL3"],  // add "${incdir}/ ${incdir}/SDL3/" to include path
  "export_cflags": ["-DSDL_SHARED"], // add "-DSDL_SHARED" to cflags
  "export_libraries": [],
  "library_name": "libSDL3"
}

In my original post, I was proposing build system dependent hooks. Properties and CMake variables are cmake concepts which has nothing to do with ndk-build.

For ndk-build, you need to be able to add information to each module before the next $(CLEAR_VARS). So this information need to be added to module.json. For CMake, all can be done in prefab.json.

I'm thinking about the following as an example.

prefab.json

{
  "schema_version": 42,
  "name": "SDL3",
  "version": "3.0.0",
  "dependencies": [],
  "buildsystem_patches": {
    "cmake": ["prefab-cmake-patches.cmake"],
    "ndk-build": ["buildsystem_patches/prefab-ndk-build-patches.mk"]
  }
}

prefab-cmake-patches.cmake would then contain the code of my initial post, which prefab then should to an appropriate location and include(). For ndk-build, you could copy past the file to the generated Android.mk.

For module dependent modifications, something similar can be done for abi.json:

{
  "export_libraries": [],
  "library_name": "libSDL3",
  "buildsystem_patches": {
    "cmake": ["SDL3-cmake-patches.cmake"],
    "ndk-build": ["SDL3-ndk-build-patches.mk"]
  }
}

I don't know how the Android.mk files look like which prefab generates, but SDL3-ndk-build-patches.mk could look like the following to add extra include dirs.

LOCAL_EXPORT_C_INCLUDES := \
    $(SDL3_INCLUDE_DIR) \
    $(SDL3_INCLUDE_DIR)/SDL3

buildsystem_patches is a bad name. Alternatives are patches, includes, hooks, ...

DanAlbert commented 1 year ago

export_includes is already supported. That's what the include directory in the module is.

The other request here seems to be export_preprocessor_definitions (or whatever name). We can't do generic cflag exports because cflags are not portable across compilers. Allowing preprocessor definition exports is pretty doable (though for your example, that seems like something that should be up to the consumer?)

If you truly do need to ship specific CMake code and not just CMake targets, I don't think prefab is the right tool for the job. There are a lot of other systems that do that (vcpkg certainly does, and I suspect hunter does as well).

madebr commented 1 year ago

export_includes is already supported. That's what the include directory in the module is.

SDL installs all headers in a SDL3 subdirectory. I want users to be able to include it as either #include <SDL3/SDL.h> or #include <SDL.h>

The other request here seems to be export_preprocessor_definitions (or whatever name). We can't do generic cflag exports because cflags are not portable across compilers. Allowing preprocessor definition exports is pretty doable (though for your example, that seems like something that should be up to the consumer?)

My example was not so good. I added it as an after thought to show its use. It's useful for projects which handle shared/static libraries as:

#if defined(MYLIB_STATIC)
#define MYLIB_EXPORT
#else
#define MYLIB_EXPORT __declspec(dllimport)
#endif
int MYLIB_EXPORT mylib_function(void);

If you truly do need to ship specific CMake code and not just CMake targets, I don't think prefab is the right tool for the job. There are a lot of other systems that do that (vcpkg certainly does, and I suspect hunter does as well).

The targets that prefab currently generates are perfectly adequate for SDL's purpose. I was just asking for the "finishing touches".

We'd like to ship ready-made aar's that people can drop into their gradle script without forcing them to use a particular external tool. Prefab's integration into Android Studio is really useful for that.

I took a peek at vcpkg's Android documentation. It looks like all cmake scripts are dropped when creating a prefab aar. So it potentially suffers from the same issue as I try to address above.

DanAlbert commented 1 year ago

SDL installs all headers in a SDL3 subdirectory. I want users to be able to include it as either #include <SDL3/SDL.h> or #include

... why? It should behave however SDL would in a typical installation (from apt or whatever). Does apt install libsdl-dev actually expose those headers with both paths?

The targets that prefab currently generates are perfectly adequate for SDL's purpose. I was just asking for the "finishing touches".

Those "finishing touches" are not appropriate for prefab though. The entire point of prefab is to be build system agnostic. If you can't describe the feature in a build system agnostic way, prefab is the wrong tool. There are other systems that handle build system specific behavior already.

If you need export_preprocessor_defs or other portable behaviors, we can convert this bug to an FR for that, but the exact request in the OP completely defeats the purpose of prefab, so is better served by another tool.

I took a peek at vcpkg's Android documentation. It looks like all cmake scripts are dropped when creating a prefab aar. So it potentially suffers from the same issue as I try to address above.

Yes, you'd need to use the normal vcpkg workflow.

madebr commented 1 year ago

SDL installs all headers in a SDL3 subdirectory. I want users to be able to include it as either #include <SDL3/SDL.h> or #include

... why? It should behave however SDL would in a typical installation (from apt or whatever). Does apt install libsdl-dev actually expose those headers with both paths?

Yes, they are made available though SDL2Config.cmake/sdl2-config.cmake/sdl2.pc that are also installed.

The sdl2.pc looks as follows. See Cflags. It contains -I${includedir} -I${includedir}/SDL2.

# sdl pkg-config source file

prefix=/usr
exec_prefix=${prefix}
libdir=${exec_prefix}/lib64
includedir=${prefix}/include

Name: sdl2
Description: Simple DirectMedia Layer is a cross-platform multimedia library designed to provide low level access to audio, keyboard, mouse, joystick, 3D hardware via OpenGL, and 2D video framebuffer.
Version: 2.26.0
Requires:
Conflicts:
Libs: -L${libdir}   -lSDL2 
Libs.private:  -lSDL2 -pthread  -lm -lrt
Cflags: -I${includedir} -I${includedir}/SDL2  -D_REENTRANT

The targets that prefab currently generates are perfectly adequate for SDL's purpose. I was just asking for the "finishing touches".

Those "finishing touches" are not appropriate for prefab though. The entire point of prefab is to be build system agnostic. If you can't describe the feature in a build system agnostic way, prefab is the wrong tool. There are other systems that handle build system specific behavior already.

I'm an android noob here. What other ways does Google provide so people can use usual cmake machinery from within gradle? I don't know of a way to embed cmake config files inside an android archive (and being used by gradle).

About being build system agnostic, I'd like to point at what conan provides. There, you can set various attributes on self.cpp_info in package_info. Using build_modules, you are able to override build system specific properties.

But I appreciate your determination on keeping prefab build system agnostic.

DanAlbert commented 1 year ago

What other ways does Google provide so people can use usual cmake machinery from within gradle? I don't know of a way to embed cmake config files inside an android archive (and being used by gradle).

I don't know about built-in AGP support for the other systems. I think vcpkg expects each developer to install packages as part of their project setup, and then they'll be available to AGP without AGP needing to get directly involved. If you need something more than that, you could file an FR against AGP.

For this particular case, you could just add the SDL repo as a git submodule and build it as part of the project. It's a CMake project, so likely doesn't have the host OS portability issues that are one of reasons prefab insists on prebuilts rather than building from source.

About being build system agnostic, I'd like to point at what conan provides. There, you can set various attributes on self.cpp_info in package_info. Using build_modules, you are able to override build system specific properties.

Thanks, it's helpful to know that conan makes an affordance for this. I'm still very wary of exposing such a feature, as doing so opens the door for people to make packages that work only in cmake. Based on this bug, if we'd included that feature, you'd have used that for SDL instead of filing an FR for export_preprocessor_defs, and the package would have just been broken for ndk-build, right? That is why I'm pushing so hard against this.

The only argument in favor of customizing the cmake output from prefab I see here is to work around missing features in prefab. Is that correct? If so, let's convert this to an FR for export_preprocessor_defs, and get another filed for export_includes. If not, what else is missing?

madebr commented 1 year ago

The only argument in favor of customizing the cmake output from prefab I see here is to work around missing features in prefab. Is that correct? If so, let's convert this to an FR for export_preprocessor_defs, and get another filed for export_includes. If not, what else is missing?

That's fine with me. With these, SDLX:: targets will behave the same as desktop ones.

DanAlbert commented 1 year ago

SGTM, thanks for working with me to narrow down the crux of the issue. I've retitled the bug to cover that (will split it into two when someone actually has the time to work on this).

Just a heads up, but it's not likely that any of the problems mentioned here will be addressed soon, as the team has some other pretty high priority work that needs to happen before we can devote any time here.