Package components - Githubissues

SirLynix commented 2 years ago

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

One issue I regularly have with xmake package is the fact there's no good way to handle libraries spanning over multiple modules (multiple components).

For example, the SFML has five libraries: sfml-system, sfml-window, sfml-graphics, sfml-audio, sfml-network. Other libraries like Qt, Boost and my own engine have a similar issue.

Let's take the case where we want to have a SFML client and a SFML server, depending on a common library.

xmake currently offers three ways to handle this:

Using add_packages links

add_requires("sfml")

target("library")
    set_kind("shared")
    add_packages("sfml", { links = { "sfml-system", "sfml-network" }, public = true })

target("server")
    add_deps("library")

target("client")
    add_packages("sfml")

Advantages:

Package is only installed once

Disavantages:

Links and syslinks have to be manually handled, which can break easily (for example, link order matters on Linux, libraries can have another name depending on debug/static, etc.)
The package has no way to handle this itself

Using package configs

add_requires("sfml~server", { configs = { audio = false, graphics = false, window = false } })
add_requires("sfml~client")

target("library")
    set_kind("shared")
    add_packages("sfml~server", public = true })

target("server")
    add_deps("library")

target("client")
    add_packages("sfml~client") -- Duplicate links?
-- Client links both sfml~server and sfml~client!

Advantages:

Package can handle syslinks and stuff that changes depending on which components are enabled

Disavantages:

SFML is compiled twice and takes up more space
client depends on sfml\~server and sfml\~client

Using multiple packages

Imagine now there was a package for each sfml component ("sfml-system", "sfml-window", etc.).

add_requires("sfml-network", "sfml-graphics", "sfml-audio")

target("library")
    set_kind("shared")
    add_packages("sfml-network", { public = true })

target("server")
    add_deps("library")

target("client")
    add_packages("sfml-graphics", "sfml-audio")

Advantages:

All targets are well built, dependencies like sfml-system and sfml-window are well handled

Disavantages:

Requires a package (like qt5base) to build SFML once and metapackages (like qt5core, qt5gui, and more) to fetch only what's needed.
From my experience with the qt5 package, this isn't straightforward and can easily break.
Would require to make a lot of packages

Describe the solution you'd like

I think we should be able to handle multiple components in the same package, installing the lib only once but allowing to select which components we want, without having to rebuild it multiple times.

First solution: handle subpackages

package("sfml")
...

    sub_package("system")
        add_links("sfml-system")
        -- also supports on_load

    sub_package("network")
        add_links("sfml-network")
        add_deps("system") -- depends on system component

Usage:

add_requires("sfml")

target("library")
    set_kind("shared")
    add_packages("sfml/network", { public = true }) -- Use component network of package sfml

target("server")
    add_deps("library")

target("client")
    add_packages("sfml/graphics", "sfml/audio") -- Use components graphics, audio, and their dependencies (along with sfml/network from library)

Advantages:

Fixes the issue, the package is built only once and can handle all syslinks and more.
Makes it very straightforward to handle libs with multiple components
If we keep add_packages("sfml") to include all components, this doesn't break any code.

Disavantages:

Probably a bit complicated to implement.

Second solution: allows some configs to not change package key

package("sfml")
    -- Note pkg_key
    add_configs("graphics",   {pkg_key = false, description = "Use the graphics module", default = true, type = "boolean"})
    add_configs("window",     {pkg_key = false, description = "Use the window module", default = true, type = "boolean"})
    add_configs("audio",      {pkg_key = false, description = "Use the audio module", default = true, type = "boolean"})
    add_configs("network",    {pkg_key = false, description = "Use the network module", default = true, type = "boolean"})

    on_load(function (package)
        -- select links and syslinks depending on configs
    end)

    on_install(function (package)
        -- compiles everything
    end)

Usage:

add_requires("sfml~server", { configs = { audio = false, graphics = false, window = false } })
add_requires("sfml~client")
-- Both are the same package

target("library")
    set_kind("shared")
    add_packages("sfml~server", public = true })

target("server")
    add_deps("library")

target("client")
    add_packages("sfml~client") -- Duplicate links?
-- Client links both sfml~server and sfml~client!

Advantages:

Doesn't install the SFML twice.
A lot simpler to implement.

Disavantages:

Doesn't fully fix the issue

This feature would make xmake way more awesome compared to conan/vcpkg (which I don't think can handle components) and could be used with libraries such as Qt, Boost and more.

I understand it may be complicated to implemented (especially the first solution), but I think it's worth it. Also the second solution can be implemented quickly to help a bit for now.

It wouldn't break compatibility if we make add_packages("xxx") select all components by default.

Describe alternatives you've considered

Described at the top.

Additional context

No response

waruqi commented 2 years ago

First solution: handle subpackages

I remember discussing this issue many times before. I will not consider it for the time being.

SirLynix commented 2 years ago

I know we discussed the issue multiple times before on Discord, but the issue remains, which is why I'm exposing solutions here as for now there's no real solution to this problem.

At least if we had a way to tell a package config doesn't change its key, it would be a bit better (and would allow to handle packages like SFML)

waruqi commented 2 years ago

cmake's find_package has something like a component, although this can be wrapped into a package component-like pattern. But it's a bit complicated to implement, and I don't have a good solution yet, nor do I have much time to think about it.

Maybe in the future, in my spare time, I'll think further about how to improve it, but it should be a while yet.

Of course, I would welcome some better solutions and design ideas to be presented here.

waruqi commented 2 years ago

package("sfml") ...

sub_package("system") add_links("sfml-system") Probably a bit complicated to implement.

The current parsing scope (target,package,rule ..) only supports a single level and cannot support nested levels, it involves the core of the parser and the complexity and risk of change is too high.

Furthermore, it is not just the implementation that is complex, we also need to ensure that the complexity of maintaining the package does not increase. Even if we support a sub-package or component model in the future, we need to design configurations that are simpler and easier to maintain.

SirLynix commented 2 years ago

I understand the scope issue, maybe something like this:

package("sfml")
...

    add_components("system", { links = "sfml-system" })
    add_components("network", { deps = { "system" }, links = "sfml-network" })

Which could handle callbacks as well:

    add_components("network", { 
        component_deps = { "system" }, 
        links = "sfml-network", 
        on_load = function (package)
            if package:is_plat("windows", "mingw") then
                package:add("syslinks", "ws32")
            end
        end
    })

It's less readable but it maintains a single-scope API.

If you don't have time for this, I could try to work on this by myself, as a proof of concept.

waruqi commented 2 years ago

It's less readable but it maintains a single-scope API.

It is still a bit complex, and there is no interface design that I am fully satisfied with yet that is simple and readable, yet flexible enough.

There are differences in the way it is implemented for different interfaces. But I still haven't figured out how to design them yet.

waruqi commented 2 years ago

I think it would be better to configure the components in a single on_load. If it's a simple component, you can avoid having to configure it in on_load at all.

package("sfml")
    add_components("graphics",   {description = "Use the graphics module", deps = "system", links = "sfml-graphics"})
    add_components("system",     {description = "Use the system module", default = true, links = "sfml-system"})
    add_components("network",    {description = "Use the network module", default = true})

    on_load(function (package)

       -- more flexible configuration components
        local network = package:component("network")
        network:add("links", "sfml-network")
        network:add("deps", "system")
        if package:is_plat("windows") then
            network:add("syslink", "ws2_32")
        end

    end)

    on_install(function (package)
        -- compiles everything
    end)

add_requires("sfml")
target("test")
     add_packages("sfml", {components = "graphics"})

SirLynix commented 2 years ago

Looks good, but how would that handle this case:

add_requires("sfml")

target("library")
    set_kind("shared")
    add_packages("sfml", { components = "network", public = true })

target("server")
    add_deps("library")

target("client")
    add_deps("library")
    add_packages("sfml", { components = { "graphics", "audio" })

Does client have network component as well?

waruqi commented 2 years ago

target("client").
 + add_deps("library")?

SirLynix commented 2 years ago

Yes sorry, I forgot about it.

With add_deps("library") would the sfml package override the components or combine them?

waruqi commented 2 years ago

Yes sorry, I forgot about it.

With add_deps("library") would the sfml package override the components or combine them?

I can't be sure now.

waruqi commented 2 years ago

There are also some other issues, if you switch to components, then the determination of whether a component is enabled is made in the actual link stage, not in on_load. add_packages("xxx", {components = "xxx"})

However, some package dependencies that depend on components must be added at the on_load stage.

        if package:config("window") or package:config("graphics") then
            if package:is_plat("linux") then
                package:add("deps", "libx11", "libxrandr", "freetype", "eudev")
                package:add("deps", "opengl", "glx", {optional = true})
            end
        end

SirLynix commented 2 years ago

I can't be sure now.

I think the safest option now would be to combine them, I don't see when you would to override them.

There are also some other issues, if you switch to components, then the determination of whether a component is enabled is made in the actual link stage, not in on_load.

I feel like even now we have this issue, with both on_load and on_fetch requiring to do the same thing (see https://github.com/NazaraEngine/xmake-repo/blob/master/packages/n/nazaraengine/xmake.lua#L144 )

waruqi commented 2 years ago

However, it is now possible to disable some deps in on_load, as the configs are configured in add_requires.

But with the components, we don't have any chance to disable some deps

SirLynix commented 2 years ago

If those deps are necessary to build the package, then they should always be there, since component feature requires a fully-built package to work from my understanding.

However we could make them private and make them public only if a component is enabled.

waruqi commented 2 years ago

If some of the components are not used in the project, this can introduce a lot of unnecessary deps.

This will make the compilation and installation slower and take up more disk space.

Therefore, we also need to add add_configs("xxx") to allow the user to disable it.

This will be.

package("sfml")
    add_components("graphics",   {description = "Use the graphics module", deps = "system", links = "sfml-graphics"})
    add_components("system",     {description = "Use the system module", default = true, links = "sfml-system"})
    add_components("network",    {description = "Use the network module", default = true})

    add_configs("graphics",   {description = "Use the graphics module", deps = "system", links = "sfml-graphics"})
    add_configs("system",     {description = "Use the system module", default = true, links = "sfml-system"})
    add_configs("network",    {description = "Use the network module", default = true})

   on_load(function (package)
        if package:config("window") or package:config("graphics") then
            if package:is_plat("linux") then
                package:add("deps", "libx11", "libxrandr", "freetype", "eudev")
                package:add("deps", "opengl", "glx", {optional = true})
            end
        end

       -- more flexible configuration components
        local network = package:component("network")
        network:add("links", "sfml-network")
        network:add("deps", "system")
        if package:is_plat("windows") then
            network:add("syslink", "ws2_32")
        end
   end)

SirLynix commented 2 years ago

Having both would indeed fix the issue for a package like SFML (plus it wouldn't break compatibility).

As for Qt (and perhaps Boost), only components would be required.

This seems like a good solution, maybe the syntax can be improved (I'm not sure how yet) to keep package simple. What you suggest seems right.

waruqi commented 2 years ago

I'll consider trying to implement it when I have time later. But it will take some time, I'm working on some c++ modules.

SirLynix commented 2 years ago

No problem, thank you for considering this!

ImperatorS79 commented 2 years ago

I will add my thoughts on this but related to external sources.

I always prefer to rely on precompiled packages. If we take as an exemple ffmpeg, we have the followings:

ffmpeg has indeed multiple components: "libavcodec", "libavdevice", "libavfilter", "libavformat", "libavutil", "libpostproc", "libswresample", "libswscale", and on top of that ffmpeg itself.
The version of the components and the version of ffmpeg are not the same (it is often implied in the find_package function for each package source that all the .pc files in a package have the same version, which is wrong).
pacman and brew store ffmpeg, its components and their .pc file in the same package. For apt however, you have multuple packages (one for each component).
It also happen that there is multiple .pc/.cmake files are in the same apt/pacman/brew/whatever package.
There is also often on apt a "meta-package" installing all the subpackages, but not always (none for ffmpeg, one for boost)
Sometimes some packages have no .pc and .cmake files but still multiple components (example: boost on msys2, but I think it is more of a msys2 problem here).

So, when adding add_extsources at the beginning of a package in xmake-repo, it should be possible to specify, for each package manager:

This package gives you all components,
This set of packages gives you one or more components each.

For example:

add_extsources("pacman::ffmpeg", {{"ffmpeg", "apt::ffmpeg"}, {"avcodec, "apt::libavcodec-dev"}, {"avdevice", "apt::libavdevice-dev"}, {"avfilter", "apt::libavfilter-dev"}, {"avformat", "apt::libavformat-dev"}, {"avutil", "apt::libavutil-dev"}, {"postproc", "apt::postproc-dev"}, {"swresample", "apt::libswresample-dev"}, {"swscale", "apt::libswscale-dev"}}, "apt::ffmpeg-meta-package-that-does-not-exist"}

This means:

on pacman, you can install one package to have everything
on apt, you can install either some of those packages, depending of what you need, or that meta-package (if any).

waruqi commented 2 years ago

I have supported it on https://github.com/xmake-io/xmake/pull/2932

Use components to control compilation and links

add_requires("sfml")

target("graphics")
    set_kind("static")
    add_files("src/graphics.cpp")
    add_packages("sfml", {components = "graphics", public = true})

target("network")
    set_kind("static")
    add_files("src/network.cpp")
    add_packages("sfml", {components = "network", public = true})

target("test")
    set_kind("binary")
    add_files("src/main.cpp")
    add_deps("graphics", "network")

Package configuration

Define components list

in description scope:

package("sfml")
    add_components("graphics", "windows", "audio", "system")

or in script scope:

package("sfml")
    on_load(function (package)
        package:add("components", "graphics", "windows", "audio", "system")
    end)

Configure the components we can use according to configs.

package("sfml")
    on_load(function (package)
        for _, component in ipairs({"graphics", "window", "audio", "network"}) do
            if package:config(component) then
                package:add("components", component)
            end
        end
    end)

configure each components

We can configure links, defines, includedirs etc. for each component individually

package("sfml")
    on_component("graphics", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-graphics" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("links", "freetype")
            component:add("syslinks", "opengl32", "gdi32", "user32", "advapi32")
        end
    end)

    on_component("window", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-window" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("syslinks", "opengl32", "gdi32", "user32", "advapi32")
        end
    end)

Components dependencies

With component dependencies, we can further simplify the use of components for the user.

for example, if we use graphics component, the windows and system components will also be enabled.

add_packages("sfml", {components = "graphics")

package("sfml")
    on_load(function (package)
        -- ...
        package:add("components", "graphics", {deps = {"window", "system"}})
    end)

Therefore, the user does not need to relate to the internal dependencies of the components, but only needs to enable the components they need.

Show package components information

Show info in current project

$ xmake require --info sfml
The package info of project:
    require(sfml): 
      -> description: Simple and Fast Multimedia Library
      -> version: 2.5.1
      ...
      -> components: 
         -> system: 
         -> graphics: system, window
         -> window: system
         -> audio: system
         -> network: system

Use xrepo to show info in global

$ xrepo info sfml

Full example

add_rules("mode.debug", "mode.release")

add_requires("sfml")

target("graphics")
    set_kind("static")
    add_files("src/graphics.cpp")
    add_packages("sfml", {components = "graphics", public = true})

target("network")
    set_kind("static")
    add_files("src/network.cpp")
    add_packages("sfml", {components = "network", public = true})

target("test")
    set_kind("binary")
    add_files("src/main.cpp")
    add_deps("graphics", "network")

package("sfml")

    set_homepage("https://www.sfml-dev.org")
    set_description("Simple and Fast Multimedia Library")

    if is_plat("windows", "linux") then
        set_urls("https://www.sfml-dev.org/files/SFML-$(version)-sources.zip")
        add_urls("https://github.com/SFML/SFML/releases/download/$(version)/SFML-$(version)-sources.zip")
        add_versions("2.5.1", "bf1e0643acb92369b24572b703473af60bac82caf5af61e77c063b779471bb7f")
    elseif is_plat("macosx") then
        if is_arch("x64", "x86_64") then
            set_urls("https://www.sfml-dev.org/files/SFML-$(version)-macOS-clang.tar.gz")
            add_versions("2.5.1", "6af0f14fbd41dc038a00d7709f26fb66bb7ccdfe6187657ef0ef8cba578dcf14")

            add_configs("debug", {builtin = true, description = "Enable debug symbols.", default = false, type = "boolean", readonly = true})
            add_configs("shared", {description = "Build shared library.", default = true, type = "boolean", readonly = true})
        end
    elseif is_plat("mingw") then
        if is_arch("x64", "x86_64") then
            set_urls("https://www.sfml-dev.org/files/SFML-$(version)-windows-gcc-7.3.0-mingw-64-bit.zip")
            add_versions("2.5.1", "671e786f1af934c488cb22c634251c8c8bd441c709b4ef7bc6bbe227b2a28560")
        elseif is_arch("x86", "i386") then
            set_urls("https://www.sfml-dev.org/files/SFML-$(version)-windows-gcc-7.3.0-mingw-32-bit.zip")
            add_versions("2.5.1", "92d864c9c9094dc9d91e0006d66784f25ac900a8ee23c3f79db626de46a1d9d8")
        end
    end

    if is_plat("linux") then
        add_syslinks("pthread")
    end

    add_configs("graphics",   {description = "Use the graphics module", default = true, type = "boolean"})
    add_configs("window",     {description = "Use the window module", default = true, type = "boolean"})
    add_configs("audio",      {description = "Use the audio module", default = true, type = "boolean"})
    add_configs("network",    {description = "Use the network module", default = true, type = "boolean"})
    if is_plat("windows", "mingw") then
        add_configs("main",       {description = "Link to the sfml-main library", default = true, type = "boolean"})
    end

    if is_plat("macosx") then
        add_extsources("brew::sfml/sfml-all")
    end

    on_component("graphics", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-graphics" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("links", "freetype")
            component:add("syslinks", "opengl32", "gdi32", "user32", "advapi32")
        end
        component:add("deps", "window", "system")
        component:add("extsources", "brew::sfml/sfml-graphics")
    end)

    on_component("window", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-window" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("syslinks", "opengl32", "gdi32", "user32", "advapi32")
        end
        component:add("deps", "system")
        component:add("extsources", "brew::sfml/sfml-window")
    end)

    on_component("audio", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-audio" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("links", "openal32", "flac", "vorbisenc", "vorbisfile", "vorbis", "ogg")
        end
        component:add("deps", "system")
        component:add("extsources", "brew::sfml/sfml-audio")
    end)

    on_component("network", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-network" .. e)
        if package:is_plat("windows", "mingw") and not package:config("shared") then
            component:add("syslinks", "ws2_32")
        end
        component:add("deps", "system")
        component:add("extsources", "brew::sfml/sfml-network")
        component:add("extsources", "apt::sfml-network")
    end)

    on_component("system", function (package, component)
        local e = package:config("shared") and "" or "-s"
        component:add("links", "sfml-system" .. e)
        if package:is_plat("windows", "mingw") then
            component:add("syslinks", "winmm")
        end
        if package:is_plat("windows", "mingw") and package:config("main") then
            component:add("deps", "main")
        end
        component:add("extsources", "brew::sfml/sfml-system")
    end)

    on_component("main", function (package, component)
        if package:is_plat("windows", "mingw") then
            local main_module = "sfml-main"
            if package:debug() then
                main_module = main_module .. "-d"
            end
            component:add("links", main_module)
        end
    end)

    on_load("windows", "linux", "macosx", "mingw", function (package)
        if package:is_plat("windows", "linux") then
            package:add("deps", "cmake")
        end

        if not package:config("shared") then
            package:add("defines", "SFML_STATIC")
        end

        if package:is_plat("linux") then
            if package:config("window") or package:config("graphics") then
                package:add("deps", "libx11", "libxext", "libxrandr", "libxrender", "freetype", "eudev")
                package:add("deps", "opengl", "glx", {optional = true})
            end
            if package:config("audio") then
                package:add("deps", "libogg", "libflac", "libvorbis", "openal-soft")
            end
        end
        package:add("components", "system")
        for _, component in ipairs({"graphics", "window", "audio", "network"}) do
            if package:config(component) then
                package:add("components", component)
            end
        end
        if package:is_plat("windows", "mingw") and package:config("main") then
            package:add("components", "main")
        end
    end)

    on_install("windows", "linux", function (package)
        local configs = {"-DSFML_BUILD_DOC=OFF", "-DSFML_BUILD_EXAMPLES=OFF"}
        table.insert(configs, "-DCMAKE_BUILD_TYPE=" .. (package:debug() and "Debug" or "Release"))
        if package:config("shared") then
            table.insert(configs, "-DBUILD_SHARED_LIBS=ON")
        else
            table.insert(configs, "-DBUILD_SHARED_LIBS=OFF")
            if package:is_plat("windows") and package:config("vs_runtime"):startswith("MT") then
                table.insert(configs, "-DSFML_USE_STATIC_STD_LIBS=ON")
            end
        end
        local packagedeps
        if package:is_plat("linux") and package:config("shared") then
            io.replace("src/SFML/Graphics/CMakeLists.txt", "target_link_libraries(sfml-graphics PRIVATE X11)",
                "target_link_libraries(sfml-graphics PRIVATE X11 Xext Xrender)", {plain = true})
            packagedeps = {"libxext", "libxrender"}
        end
        table.insert(configs, "-DSFML_BUILD_AUDIO=" .. (package:config("audio") and "ON" or "OFF"))
        table.insert(configs, "-DSFML_BUILD_GRAPHICS=" .. (package:config("graphics") and "ON" or "OFF"))
        table.insert(configs, "-DSFML_BUILD_WINDOW=" .. (package:config("window") and "ON" or "OFF"))
        table.insert(configs, "-DSFML_BUILD_NETWORK=" .. (package:config("network") and "ON" or "OFF"))
        import("package.tools.cmake").install(package, configs, {packagedeps = packagedeps})
    end)

    on_install("macosx", "mingw", function (package)
        os.cp("lib", package:installdir())
        os.cp("include", package:installdir())
        if package:is_plat("mingw") then
            os.cp("bin/*", package:installdir("lib"), {rootdir = "bin"})
        end
    end)

    on_test(function (package)
        assert(package:check_cxxsnippets({test = [[
            void test(int args, char** argv) {
                sf::Clock c;
                c.restart();
            }
        ]]}, {includes = "SFML/System.hpp"}))
        if package:config("graphics") then
            assert(package:check_cxxsnippets({test = [[
                void test(int args, char** argv) {
                    sf::Text text;
                    text.setString("Hello world");
                }
            ]]}, {includes = "SFML/Graphics.hpp"}))
        end
        if package:config("window") or package:config("graphics") then
            assert(package:check_cxxsnippets({test = [[
                void test(int args, char** argv) {
                    sf::Window window(sf::VideoMode(1280, 720), "Title");

                    sf::Event event;
                    window.pollEvent(event);
                }
            ]]}, {includes = "SFML/Window.hpp"}))
        end
        if package:config("audio") then
            assert(package:check_cxxsnippets({test = [[
                void test(int args, char** argv) {
                    sf::Music music;
                    music.openFromFile("music.ogg");
                    music.play();
                }
            ]]}, {includes = "SFML/Audio.hpp"}))
        end
        if package:config("network") then
            assert(package:check_cxxsnippets({test = [[
                void test(int args, char** argv) {
                    sf::UdpSocket socket;
                    socket.bind(54000);

                    char data[100];
                    std::size_t received;
                    sf::IpAddress sender;
                    unsigned short port;
                    socket.receive(data, 100, received, sender, port);
                }
            ]]}, {includes = "SFML/Network.hpp"}))
        end
    end)
package_end()

SirLynix commented 2 years ago

this looks absolutely amazing.

maybe instead of private component, components deps could be considered? as for example with the SFML, "graphics" requires "window" which requires "system".

add_components("graphics", { deps = { "window" })
add_components("audio", "network", "window", { deps = "system" })
add_components("system")

component dependencies could be supported with a simple algorithm:

1) enabling a component adds their dependencies in the enabled component list recursively 2) components are de-duplicated

since component order doesn't matter this can be easily handled.

anyway this is really great and I will test it as soon as I can (I don't have much time lately)

waruqi commented 2 years ago

this looks absolutely amazing.

maybe instead of private component, components deps could be considered? as for example with the SFML, "graphics" requires "window" which requires "system".
add_components("graphics", { deps = { "window" })
add_components("audio", "network", "window", { deps = "system" })
add_components("system")
component dependencies could be supported with a simple algorithm:

enabling a component adds their dependencies in the enabled component list recursively

components are de-duplicated

since component order doesn't matter this can be easily handled.

anyway this is really great and I will test it as soon as I can (I don't have much time lately)

I don't think this is necessary, we just need to control the dependencies between all the components by the order in which they are added to add_components. like add_links

We should avoid introducing unnecessary complexity where possible, as introducing components is already quite complicated.

SirLynix commented 2 years ago

I understand, but that means the user has to know how dependencies are related. in your example

add_packages("sfml", { components = "graphics" })

will only link "graphics" and "system" components (because system is private), it won't link "window" component and cause a link error at usage which won't be very user-friendly.

is there a way to detect this and trigger an error message in the package? to tell the user that "window" component is required by "graphics".

SirLynix commented 2 years ago

thinking about it, if we make a Qt package using components (which is a pretty good candidate I think, as all Qt libs are installed), this means we will have to write

add_packages("qt5", { components = { "core", "gui", "widgets", "network" } })

instead of

add_packages("qt5", { components = { "widgets", "network" } }) -- link error

I understand you don't want to make it too complicated from the beginning (and truly, package components seems like an awesome feature, thanks a lot!) but I think this may be a common error for packages using components, so it seems interesting to at least be capable of handling this as an explicit error instead of a link error.

waruqi commented 2 years ago

I have supported it, try it again.

        package:add("components", "system")
        for _, component in ipairs({"graphics", "window", "audio", "network"}) do
            if package:config(component) then
                local deps = {}
                table.insert(deps, "system")
                if component == "graphics" then
                    table.insert(deps, "window")
                end
                if package:is_plat("windows", "mingw") and package:config("main") then
                    table.insert(deps, "main")
                end
                package:add("components", component, {deps = deps})
            end
        end
        if package:is_plat("windows", "mingw") and package:config("main") then
            package:add("components", "main")
        end

waruqi commented 2 years ago

I will add my thoughts on this but related to external sources.

I always prefer to rely on precompiled packages. If we take as an exemple ffmpeg, we have the followings:

ffmpeg has indeed multiple components: "libavcodec", "libavdevice", "libavfilter", "libavformat", "libavutil", "libpostproc", "libswresample", "libswscale", and on top of that ffmpeg itself.

The version of the components and the version of ffmpeg are not the same (it is often implied in the find_package function for each package source that all the .pc files in a package have the same version, which is wrong).

pacman and brew store ffmpeg, its components and their .pc file in the same package. For apt however, you have multuple packages (one for each component).

It also happen that there is multiple .pc/.cmake files are in the same apt/pacman/brew/whatever package.

There is also often on apt a "meta-package" installing all the subpackages, but not always (none for ffmpeg, one for boost)

Sometimes some packages have no .pc and .cmake files but still multiple components (example: boost on msys2, but I think it is more of a msys2 problem here).

So, when adding add_extsources at the beginning of a package in xmake-repo, it should be possible to specify, for each package manager:

This package gives you all components,

This set of packages gives you one or more components each.

For example:
add_extsources("pacman::ffmpeg", {{"ffmpeg", "apt::ffmpeg"}, {"avcodec, "apt::libavcodec-dev"}, {"avdevice", "apt::libavdevice-dev"}, {"avfilter", "apt::libavfilter-dev"}, {"avformat", "apt::libavformat-dev"}, {"avutil", "apt::libavutil-dev"}, {"postproc", "apt::postproc-dev"}, {"swresample", "apt::libswresample-dev"}, {"swscale", "apt::libswscale-dev"}}, "apt::ffmpeg-meta-package-that-does-not-exist"}
This means:

on pacman, you can install one package to have everything

on apt, you can install either some of those packages, depending of what you need, or that meta-package (if any).

I have supported to find them from components/extsources, but it only support brew now. I will continue to support apt later.

package("sfml")

    if is_plat("macosx") then
        add_extsources("brew::sfml/sfml-all")
    end

    on_component("graphics", function (package, component)
        -- ...
        component:add("extsources", "brew::sfml/sfml-graphics")
    end)

    on_component("window", function (package, component)
        -- ...
        component:add("extsources", "brew::sfml/sfml-window")
    end)

SirLynix commented 2 years ago

I just tested it, it works pretty well

package("nazaraengine")
-- ...
    add_components("widgets", { deps = "graphics" })
    add_components("graphics", { deps = "renderer" })
    add_components("renderer", { deps = {"platform", "utility"} })
    add_components("platform", { deps = "utility" })
    add_components("audio", "network", "physics2d", "physics3d", "utility", { deps = "core" })
    add_components("core")

    local function BuildLink(package, module)
        local prefix = "Nazara"
        local suffix = package:config("shared") and "" or "-s"
        if package:debug() then
            suffix = suffix .. "-d"
        end

        return prefix .. module .. suffix
    end

    local regularComponents = {"Audio", "Graphics", "Network", "Physics2D", "Physics3D", "Platform", "Utility", "Widgets"}
    for _, componentName in ipairs(regularComponents) do
        on_component(componentName:lower(), function (package, component)
            component:add("links", BuildLink(package, componentName))
        end)
    end

    on_component("core", function (package, component)
        component:add("links", BuildLink(package, "Core"))
        if is_plat("linux") then
            component:add("syslinks", "pthread")
        end
    end)

    on_component("renderer", function (package, component)
        component:add("links", BuildLink(package, "Renderer"))
        if package:is_plat("windows", "mingw") then
            component:add("syslinks", "gdi32")
            component:add("syslinks", "user32")
            component:add("syslinks", "advapi32")
        end
    end)

target("Bomberman")
    set_kind("binary")
    add_headerfiles("src/Client/**.hpp", "src/Client/**.inl", "src/Shared/**.hpp", "src/Shared/**.inl")
    add_files("src/Client/**.cpp", "src/Shared/**.cpp")
    add_packages("nazaraengine", { components = { "audio", "graphics", "network", "physics3d", "widgets" }})

target("BombermanServer")
    set_kind("binary")
    add_headerfiles("src/Server/**.hpp", "src/Server/**.inl", "src/Shared/**.hpp", "src/Shared/**.inl")
    add_files("src/Server/**.cpp", "src/Shared/**.cpp")
    add_packages("nazaraengine", { components = { "network", "physics3d", "utility" }})

Pretty awesome!

What's the use of having on_component as a function? is it called exclusively when a component is enabled?

waruqi commented 2 years ago

What's the use of having on_component as a function? is it called exclusively when a component is enabled?

In order to better isolate the configuration of each component, it is clearer and more readable to maintain, rather than all in the on_load configuration. However, it is equivalent to on_load_component, but with a more concise name.

add_components("widgets", { deps = "graphics" })

maybe I will use component:add("deps",.. instead of it in on_component.

This is probably better and keeps the style consistent with other add_deps interfaces.

SirLynix commented 2 years ago

Alright, this will complicate a bit my implementation of the nazaraengine package (since I will have to provide a custom on_component for more modules) but it's not a big deal. or perhaps there is a better way (which would take less lines) to do that with the current interface? Maybe a table which would then be parsed in on_load.

For example, if you move deps to component:add("deps"), here's what I could do:

    local components = {
        { 
            name = "Widgets",
            deps = { "graphics" } 
        },
        { 
            name = "Graphics", 
            deps = { "graphics" } 
        },
        { 
            name = "Renderer", 
            deps = { "platform", "utility" },
            custom = function (package, component)
                if package:is_plat("windows", "mingw") then
                    component:add("syslinks", "gdi32", "user32", "advapi32")
                end        
            end
        },
        { 
            name = "Platform", 
            deps = { "utility" } 
        },
        { 
            name = "Audio", 
            deps = { "core" } 
        },
        { 
            name = "Network", 
            deps = { "core" } 
        },
        { 
            name = "Physics2D", 
            deps = { "core" } 
        },
        { 
            name = "Physics3D", 
            deps = { "core" } 
        },
        { 
            name = "Utility", 
            deps = { "core" } 
        },
        {
            name = "Core",
            custom = function (package, component)
                if package:is_plat("linux") then
                    component:add("syslinks", "pthread", "dl")
                end        
            end
        }
    }

    for _, comp in ipairs(components) do
        add_components(comp.name:lower())

        on_component(comp.name:lower(), function (package, component)
            local prefix = "Nazara"
            local suffix = package:config("shared") and "" or "-s"
            if package:debug() then
                suffix = suffix .. "-d"
            end

            component:add("links", prefix .. comp.name .. suffix)
            if comp.custom then
                comp.custom(package, component)
            end
        end)
    end

which is probably more maintenable than:

    add_components("widgets", "graphics", "renderer", "platform", "physics2d", "physics3d", "network", "utility", "core")

    local function BuildLink(package, moduleName)
        local prefix = "Nazara"
        local suffix = package:config("shared") and "" or "-s"
        if package:debug() then
            suffix = suffix .. "-d"
        end

        return prefix .. moduleName .. suffix
    end

    on_component("widgets", function (package, component)
        component:add("deps", "graphics")
        component:add("links", BuildLink(package, "Widgets"))
    end)
    on_component("graphics", function (package, component)
        component:add("deps", "renderer")
        component:add("links", BuildLink(package, "Graphics"))
    end)
    on_component("renderer", function (package, component)
        component:add("deps", "platform", "utility")
        component:add("links", BuildLink(package, "Renderer"))
        if package:is_plat("windows", "mingw") then
            component:add("syslinks", "gdi32", "user32", "advapi32")
        end
    end)
    on_component("platform", function (package, component)
        component:add("deps", "utility")
        component:add("links", BuildLink(package, "Platform"))
    end)
    on_component("audio", function (package, component)
        component:add("deps", "core")
        component:add("links", BuildLink(package, "Audio"))
    end)
    on_component("network", function (package, component)
        component:add("deps", "core")
        component:add("links", BuildLink(package, "Network"))
    end)
    on_component("physics2d", function (package, component)
        component:add("deps", "core")
        component:add("links", BuildLink(package, "Physics2D"))
    end)
    on_component("physics3d", function (package, component)
        component:add("deps", "core")
        component:add("links", BuildLink(package, "Physics3D"))
    end)
    on_component("utility", function (package, component)
        component:add("deps", "core")
        component:add("links", BuildLink(package, "Utility"))
    end)
    on_component("core", function (package, component)
        component:add("links", BuildLink(package, "Core"))
        if package:is_plat("linux") then
            component:add("syslinks", "pthread", "dl")
        end
    end)

I'm not even sure of which one I like the most, maybe you have a better idea to handle this?

It's not a big deal if I have to do this in a package, my package is a bit complicated anyway.

waruqi commented 2 years ago

If you are already using on_component, then continuing to use component:add("deps") doesn't make it any more complicated, and may even simplify the configuration further.

diff +8 -9 lines

https://github.com/xmake-io/xmake/pull/2932/commits/317f886c305b6b51f3ad0199b26b8f8a160e4a21

And if you don't want to use on_component, you can also configure them directly in on_load, where on_component is optional.

package("sfml")
    add_components("platform")
    add_components("audio", "network", "physics2d", "physics3d", "utility")
    add_components("core")

    on_load(function (package)
          for _, name in ipairs({"audio", "network", "physics2d", "physics3d", "utility"}) do
              package:component(name):add("deps", "core")
          end
    end)

Or you can also handle all components in single on_component.

package("sfml")
    add_components("platform")
    add_components("audio", "network", "physics2d", "physics3d", "utility")
    add_components("core")

    on_component(function (package, component)
          component:add("deps", "core")
          if component:name() == "audio" then
              -- ..
          end
    end)

BTW, I have kept the add_components("", {deps = ""}) configuration and you can still continue to use it.

waruqi commented 2 years ago

I have merged this patch.

waruqi commented 2 years ago

I have updated sfml package to support components in xmake-repo

xmake-io / xmake

Package components #2636

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

Using add_packages links

Using package configs

Using multiple packages

Describe the solution you'd like

First solution: handle subpackages

Second solution: allows some configs to not change package key

Describe alternatives you've considered

Additional context

Use components to control compilation and links

Package configuration

Define components list

configure each components

Components dependencies

Show package components information

Full example