[stb] Use stb's STB_XXX_IMPLEMENTATION macros and have real stb implementation files, and have seperate stb implementations

Cazadorro commented 6 months ago

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

Note, I'm planning on doing the work for this anyway in a personal vcpkg registry regardless of how people feel, but this is a fairly big change so I can't just make a pull request for this.

stb is not a real "header only library" yet, vcpkg treats as if it is in the port. Most files require STB_XXX_IMPLEMENTATION inside only in a single source file to work properly, it's not a simple include process. It's also not a single library, it's several separate libraries that don't have dependencies on one another. I propose that instead of the current set up, the port for stb get split into

stb-vorbis               
stb-hexwave              
stb-image                
stb-truetype             
stb-image-write          
stb-image-resize2        
stb-rect-pack            
stb-perlin               
stb-ds                   
stb-sprintf              
stb-textedit             
stb-voxel-render         
stb-dxt                  
stb-easy-font            
stb-tilemap-editor       
stb-herringbone-wang-tile
stb-c-lexer              
stb-divide               
stb-connected-components 
stb-leakcheck            
stb-include

and each properly deal with STB_XXX_IMPLEMENTATION

Proposed solution

Each of these ports would use fetch content/fiile download/external project add to grab the relevant files. Features could then be enabled for each relevant library with features (for example, stb-vorbis), and each would be able to be used via stb::image stb::truetype etc... in the relevant cmake file. As it stands, the naive solution with out VCPKG is actually easier to manage stb than vcpkg's solution is.

Describe alternatives you've considered

We could instead add "features" to stb which would include each of the relevant libraries, but this would break anybody who's relying on stb right now. The change above would not necessitate a change to the stb port itself, besides maybe deprecating it if need be. Additionally, there libraries aren't even features. Unlike a regular repo, each of the stb_files.h should really be their own repo and project, it's just organized as one repo.

Additional context

Our current code depends on the whole STB_XXX_IMPLEMENTATION issue being fixed, so that we don't have to worry about including stb_xxx.h files in more than one place causing issues. With out changes, it's impossible to use VCPKG's publically availible stb port in our codebase.

Cazadorro commented 6 months ago

Okay, I've got a proof of concept of how I would attempt this, I managed to replicate this in my private registry,

First, each port has the following file structure:

port/
    stb-image/
        cmake/
            config.cmake.in
        CMakeLists.txt
        portfile.cmake
        vcpkg.json
    stb-hexwave/ ... 
    stb-image-write/ ...

config.cmake.in contains the typical:

@PACKAGE_INIT@

include("${CMAKE_CURRENT_LIST_DIR}/stb-imageTargets.cmake")

include(CMakeFindDependencyMacro)

check_required_components(stb-image)

CMakeLists.txt likewise acts like a normal CMakeLists.txt, except we create the implementation file for the given header (note version is derived from the table in the repo, as the author doesn't use tags, even further showing the balkanization of the header files there):

cmake_minimum_required(VERSION 3.26)
project(stb-image)

if(NOT APPLE)
    set(CMAKE_INSTALL_RPATH $ORIGIN)
endif()

file(WRITE ${CMAKE_CURRENT_SOURCE_DIR}/stb_image.c
        "#define STB_IMAGE_IMPLEMENTATION
         #include \"stb_image.h\"")

add_library(stb-image)

#https://discourse.cmake.org/t/installing-headers-the-modern-way-regurgitated-and-revisited/3238/3
#https://crascit.com/wp-content/uploads/2019/09/Deep-CMake-For-Library-Authors-Craig-Scott-CppCon-2019.pdf
target_sources(stb-image PUBLIC
        FILE_SET HEADERS
        TYPE HEADERS
        FILES
        #BASE_DIRS .... not needed if we have no "base dirs" to treat as root dir for include purposes
        stb_image.h
)

target_sources(stb-image
        PRIVATE
        stb_image.c
)

target_include_directories(stb-image PUBLIC
        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}>
        $<BUILD_INTERFACE:${CMAKE_CURRENT_BINARY_DIR}>
        $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>
)

set_target_properties(stb-image PROPERTIES
        SOVERSION 2
        VERSION 2.29.0)

add_library(stb::image ALIAS stb-image)

set_target_properties(stb-image PROPERTIES EXPORT_NAME image)

include(CMakePackageConfigHelpers)

include(GNUInstallDirs)

install(TARGETS stb-image
        EXPORT stb-image_Targets
        FILE_SET HEADERS DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
        COMPONENT stb-image_Development
        INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}
        RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
        COMPONENT stb-image_RunTime
        LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
        COMPONENT stb-image_RunTIme
        NAMELINK_COMPONENT stb-image_Development
        ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
        COMPONENT stb-image_Development
)
set(stb-image_INSTALL_CMAKEDIR
        ${CMAKE_INSTALL_DATADIR}/stb-image
        CACHE STRING "Path to stb-image cmake files"
)

install(EXPORT stb-image_Targets
        DESTINATION ${stb-image_INSTALL_CMAKEDIR}
        NAMESPACE stb::
        FILE stb-imageTargets.cmake
        COMPONENT stb-image_Development
)

configure_package_config_file(${CMAKE_CURRENT_SOURCE_DIR}/cmake/config.cmake.in
        "${CMAKE_CURRENT_BINARY_DIR}/stb-imageConfig.cmake"
        INSTALL_DESTINATION ${stb-image_INSTALL_CMAKEDIR}
)

get_target_property(stb-image_VERSION stb-image VERSION)

write_basic_package_version_file(stb-imageConfigVersion.cmake VERSION ${stb-image_VERSION} COMPATIBILITY SameMajorVersion)

install(FILES
        ${CMAKE_CURRENT_BINARY_DIR}/stb-imageConfig.cmake
        ${CMAKE_CURRENT_BINARY_DIR}/stb-imageConfigVersion.cmake
        DESTINATION ${stb-image_INSTALL_CMAKEDIR}
)

Then portfile.cmake contains:

vcpkg_download_distfile(
        ARCHIVE
        URLS "https://raw.githubusercontent.com/nothings/stb/master/stb_image.h"
        FILENAME "stb_image.h"
        SHA512 92EA97568AE2466249DBDCDC4BD378AA3A594BBAF9FA6F21C112F1BC617035BF2B03DB1661A35CF7169E066329E43CE93E184A314A7833F57F5E3B40EE9F0B65
)

vcpkg_noextract_source_archive(
        SOURCE_PATH
        ARCHIVE "${ARCHIVE}"
        NO_REMOVE_ONE_LEVEL
)

 file(COPY "${CMAKE_CURRENT_LIST_DIR}/CMakeLists.txt" DESTINATION "${SOURCE_PATH}")
 file(COPY "${CMAKE_CURRENT_LIST_DIR}/cmake" DESTINATION "${SOURCE_PATH}")

 vcpkg_configure_cmake(
         SOURCE_PATH "${SOURCE_PATH}"
         PREFER_NINJA
 )

 vcpkg_install_cmake()
 vcpkg_fixup_cmake_targets()
 file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/include")

note I created a new function called vcpkg_noextract_source_archive because I couldn't figure out what function would allow me to just create a source path based on the file I used and not force it to extract on it, and to generate a proper ${SOURCE_PATH} which would automatically be removed. I just edited vcpkg_extract_source_archive and turned the tar -xfz command into a CMake copy command.

 vcpkg_execute_required_process(
         ALLOW_IN_DOWNLOAD_MODE
         COMMAND "${CMAKE_COMMAND}" -E copy "${arg_ARCHIVE}" "${temp_dir}"
         WORKING_DIRECTORY "${temp_dir}"
         LOGNAME extract
 )

and left everything else the same.

Finally my vcpkg.json looks like this:

{
  "name": "stb-image",
  "version-string": "2.29.0",
  "homepage": "https://github.com/nothings/stb",
  "dependencies": [
    {
      "name": "vcpkg-cmake",
      "host": true
    },
    {
      "name": "vcpkg-cmake-config",
      "host": true
    }
  ]
}

Note I think I'm using the outdated versions of vcpkg-cmake-config functions etc, so i would probably upgrade to those, but for now I was just trying to get a proof of concept working. Additionally, if there really is no base way to handle just file downloads in VCPKG's built in commands that manage ${SOURCE_PATH}, then I would probably add a new CMake host vcpkg dependency to the vpckg.json here allowing me to use my vcpkg_noextract_source_archive function.

This same process would then be repeated nearly exactly for every single other library in the list above, except for stb-vorbis because it actually has the opposite problem as the other libraries, and neesd to be turned into a header only library instead. See this: http://nothings.org/stb_vorbis/

You can also avoid having to cut-and-paste the header file with new releases by doing the following:
create 'stb_vorbis.h'
#define STB_VORBIS_HEADER_ONLY
#include "stb_vorbis.c"

In addition those files with features (like vorbis) would have the features added.

I'm also missing the LISCENSE installation which would be duplicated per library in this instance, I just didn't bother yet with it.

I have no problem giving this treatment to the rest of the libraries, but I'll pause my work until I know it's useful here, since I only actually use stb_image and stb_image_write

dg0yt commented 6 months ago

It is not entirely clear to me what you try to solve.
Why to split the port?
Why to not accept upstream's design?

AFAIU there are two use cases to consider:

stb used in ports: vcpkg wants to devendor (outdated/redundant) copies. Modifications to stb make this harder.
stb used in user projects. Modifications to stb make it harder to follow upstream's instructions. And they have a lock-in effect.

FTR you don't need vcpkg_noextract_source_archive when you have file(COPY ...). But I have the strong feeling that the package manager should handle whole packages and libraries, not single headers or objects.

Cazadorro commented 6 months ago

Why to split the port?

I'm sorry if I wasn't clear. Stb is not one library, with a collection of functionality, it's a collection of completely independent libraries that are stored in the same repo. Stb has traditionally expected people to copy and paste headers around, not even use git, cmake, or a package manager. You can see this in the instructions they give to use the library. Stb is just an outlier among projects like this, I don't know of any other ports for vcpkg that are handled like this.

Importantly, there's zero way to properly version the repo as a consequence of this design. It isn't tagged, and there are no repo versions, there are library versions for each individual header, see the table in the readme. That alone makes it a non starter to have a single port for the library, there's no way to individually handle semver compatiibility for the whole repo. For example, stb_truetype.h is on version 1.26, stb_image.h is on version 2.29, and stb_herringbone_wang_tile.h is on 0.7. How do you even represent version bumps with stb? Well the port doesn't actually deal with that at all. They just use version-dates, but that's a lie to how the repo actually represents the versions, there are proper semvers, and anyone moving to VCPKG to use these libraries loses that entirely.

And while not a giant concern at the moment, this also has consequences on compiler definitions and feature sets. If you consider the whole repo as a single library, there's no guarantee that the library author won't create a pre processor definition that is incompatible between files, they are separate libraries after all. If you have different libs for each library in the repo, you can set private target_compile_definitions to fix that.

Why to not accept upstream's design?

Sorry this is long, but I don't really know how to explain it any terser.

Stb's libs claim to be "header only libraries". However, this is not really true. If you include the header with out the proper procedure, by default, you'll run into compiler errors. The header itself basically contains a giant define block of implementation that isn't inlined. If you read the comment at the begining of the stb headers, for example, stb_image.h, it says:

   Do this:
      #define STB_IMAGE_IMPLEMENTATION
   before you include this file in *one* C or C++ file to create the implementation.

   // i.e. it should look like this:
   #include ...
   #include ...
   #include ...
   #define STB_IMAGE_IMPLEMENTATION
   #include "stb_image.h"

Which means that I've got to dedicate a special way to do things just for stb_image.h, and it's like this for pretty much every single stb_header.h. Practically what this actually means, in order to actually have a proper target for a given stb library and not have to do special stuff for each header, I'm adding a target just for stb image with it's own .c file like this:

add_library(stb-image STATIC)

# either do this, or just straight up write our on stb_image.c file. 
file(WRITE ${CMAKE_CURRENT_SOURCE_DIR}/stb_image.c
        "#define STB_IMAGE_IMPLEMENTATION
         #include \"stb_image.h\"")

target_sources(stb-image PRIVATE
        stb_image.cpp)

target_include_directories(stb-image PUBLIC ./)

add_library(stb::image  ALIAS stb-image)

And I'm doing that for every stb library I use. Note the port doesn't even expose a proper target here anyway, it just tells me to use ${Stb_INCLUDE_DIR}! And the work for this target is way less than what I'd deal with If I care about installing my own libraries, while also exposing stb's headers and stuff, which can get whole lot more complicated.

It's also not like VCPKG doesn't go in and make their own "proper" targets in other ports either (actually I'd say that the stb port is an outlier in terms of that from what I can tell), for example ImGui I believe is still in CMake limbo on the actual ImGui repo, but is properly implemented in VCPKG.

FTR you don't need vcpkg_noextract_source_archive when you have file(COPY ...)

But how do I get a proper ${SOURCE_PATH} otherwise? I'm sorry if I didn't make that clear, but that was the reason I made that function to begin with. ${SOURCE_PATH} is a black box to me, I can see where it eventually placed things inside of buildtrees, but It seemed that not only was VCPKG doing special set-up and remove procedures with the source path they generated, but that it couldn't be an arbitrary location or have an arbitrary name, and I didn't know of any functions that did those parts for me outside of the functions that already existed.

dg0yt commented 6 months ago

I'm still not convinced that it is so hard to follow upstream's design. We are talking about two lines in one (existing) source file (#define,#include), and possible adding this (new) file to your project's source file list. There is no need to have a separate library, and there is no need to generate the source file from CMake.

Personally, I'm more concerned about stb's approach leading to the same functions being compiled into multiple libraries.

If you want to elaborate your proposal:

Versioning of components is a sound reason to split ports. (Abstract concerns about conflicting flags are too vague IMO.)

SOURCE_PATH is just a variable pointing to the project source. We do have ports which set it to a subdir of the port dir. This should work for also for your proposed ports:

Offer CMakeList.txt and a C file which compiles the implementation.
Assuming a common pattern, it might even be possible to have a generic CMakeLists.txt and to pass the stb component name as a CMake option.
Pass the download location of the stb header via a CMake option.
Let your cmake project copy the downloaded file into the binary dir, and use this dir as an include directory.

Given that this is not upstream's offer, the exported cmake config should use the unofficial prefix for CMake package name and targets, e.g. package unofficial-stb-image and target unofficial::stb::image. (This is a variation of the maintainer guidelines if the actual port is stb-image, but it seems reasonable with regard to the common stb origin.)

Port stb should probably continue to exist a meta-port. It might even be possible to incrementally migrate individual components. stb could only install (own) headers which don't have an implementation.

This could be formed into a proof-of-concept for stb-image.

Disclaimer: I'm only a community member.

xiaozhuai commented 6 months ago

@Cazadorro I once tried to impletment what you said. But finally I give it up. See https://github.com/nothings/stb/blob/master/stb_connected_components.h It need also define STBCC_GRID_COUNT_X_LOG2 and STBCC_GRID_COUNT_Y_LOG2 But you cannot detemine it in vcpkg. And the same thing exists not only for stb_connected_components.

And people using the stb libraries are used to defining STB_XXX_IMPLEMENTATION macros. If vcpkg introduces this change, it will undoubtedly cause trouble to downstream users. Users need to remove #define STB_XXX_IMPLEMENTATION in their code to make things work, otherwise it will throw redefinition link errror.

So I think maybe the best way is staying quo.

Whatever, I think your method is undoubtedly an improvement. But sometimes the baggage of history is heavy. I'd rather following upstream's design, even if there is something unreasonable.

Cazadorro commented 6 months ago

@xiaozhuai I don't find the "used to defining STB_XXX_IMPELEMENTATION" argument very convincing, far more people are used to never doing that at all, and at least from personal experience, not handling this has already blocked adoption of this package from vcpkg for several people.

Additionally With what @dg0yt said, if we just use unofficial::stb::image and keep stb anyway, then nothing breaks for people who already use stb, also while the STBCC_GRID_COUNT_X_LOG2 stuff is strange, I don't think we should throw the baby out with the bathwater here, this doesn't effect any of the other independent libraries. If we can't find a way to variable-tize that (which I think we can, with VCPKG_CMAKE_CONFIGURE_OPTIONS) we could ignore it, or set a reasonable default (for example, it appears the vast majority use 10 as the default AFAIK), or have a feature for individual numbers.

dg0yt commented 6 months ago

not handling this has already blocked adoption of this package from vcpkg for several people.

Well, maybe stb isn't the right package for these users. There is nothing wrong with that.
But it might be wrong to promise simplified use when the "reasonable default" can lead to unexpected trouble (in terms of memory, run time or precision.

xiaozhuai commented 6 months ago

If we can't find a way to variable-tize that (which I think we can, with VCPKG_CMAKE_CONFIGURE_OPTIONS) we could ignore it, or set a reasonable default (for example, it appears the vast majority use 10 as the default AFAIK), or have a feature for individual numbers.

The key problem is that people who already defined STBCC_GRID_COUNT_X_LOG2 find it not works at all. This is a complete departure from upstream design.

Cazadorro commented 6 months ago

@xiaozhuai How about "source_included" as a feature that the user has to enable?

xiaozhuai commented 6 months ago

@xiaozhuai How about "source_included" as a feature that the user has to enable?

@Cazadorro I didn't get it.

In fact, I've always wanted to do what you're doing now. But I really haven't thought of a good way to resolve the problem.

Cazadorro commented 6 months ago

@xiaozhuai You can add "features" to VCPKG ports, like how OpenCV has different features like "cuda" that enable you to specify you want to use them in your vcpkg.json

"dependencies": [
    "foo",
    "bar",
    { "name": "opencv4", "features": ["cuda", "png"] }
  ]

You could do the same thing to the split stb ports.

So what I'm suggesting to reiterate:

first, zero changes to stb itself. No changes. Nothing changes about stb in vcpkg. Any talk of backwards compatibility breaking is simply not true.
second I'm suggesting is that there be split versions of stb , so with @dg0yt's suggested names, unofficial-stb-image, unofficial-stb-c-lexer.
Third, I'm suggesting that, by default the only thing that changes between stb and these unofficial library ports is that the fact you get proper semver versioning and only are given the relevant header for your library. If you use the library like below you will still have to use STBCC_GRID_COUNT_X_LOG2, STBCC_GRID_COUNT_Y_LOG2, STB_CONNECTED_COMPONENTS_IMPLEMENTATION and similar for other stb libs yourself. A project that switched to this would have zero comatibility issues if they used stb previously, and made sure the added the correct library to their dependency.

"dependencies": [
    "unofficial-stb-connected-components"
    "unofficial-stb-image",
    "unofficial-stb-image-write",
  ]

   //in some CPP file
  #define STB_IMAGE_IMPELEMENTATION
  #include "stb_image.h"

Fourth, I'm suggesting that a feature gets added to each library where possible so that you don't have to use STB_XXX_IMPLEMENTATION, yourself, making stb_xxx.h headers work like regular headers. For example the following would enable this:

"dependencies": [
// we don't have to have this feature on every port
"unofficial-stb-connected-components",
// but where it does work, we can just use it. 
{"name":"unofficial-stb-image", "features" : ["source_included"]}
{"name":"unofficial-stb-image-write", "features" : ["source_included"]}
]

//No Longer needed, so we comment out
// #define STB_IMAGE_IMPELEMENTATION
#include "stb_image.h"

dg0yt commented 6 months ago

I don't think that the proposed use of "features" is acceptable.
And I still think you propose a kind of stb fork which shouldn't really be maintained in vcpkg.

Cazadorro commented 6 months ago

I don't think that the proposed use of "features" is acceptable.

Why?

maxime-modulopi commented 1 month ago

I too believe that stb libraries should at least each have their own vcpkg port.

As stated earlier, https://github.com/nothings/stb is not a library, it is a collection of libraries, so it doesn't make sense to import the whole collection as a single port. It's basically the same situation as Boost, yet each Boost library has its own dedicated port and the boost port is just a meta-port that references all Boost library ports.

For the STB_XXX_IMPLEMENTATION question, should we expose two targets, one precompiled and one header-only, as it is done for some other header-only ports (such as glm)?

microsoft / vcpkg