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

Package dependencies are not transitive #5745

Closed
SirLynix opened this issue Oct 21, 2024 · 22 comments
Closed

Package dependencies are not transitive #5745

SirLynix opened this issue Oct 21, 2024 · 22 comments

Comments

@SirLynix
Copy link
Member

SirLynix commented Oct 21, 2024

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

Hello,

As described here, the way package dependencies are handled can cause subtle bugs.

For example

add_requires("libsdl 2.28", "libsdl_ttf")

this will install two different versions of libsdl, because libsdl_ttf doesn't specify a version:

checking for Microsoft Visual Studio (x64) version ... 2022
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
  -> libsdl 2.28.5 [runtimes:"MT"]
  -> libsdl#1 2.30.8 [runtimes:"MT", from:libsdl_ttf]
  -> libsdl_ttf 2.22.0 [runtimes:"MT"]
please input: y (y/n/m)

this may duplicate symbols, fail compilation or even worse, introduce subtle bugs that will just prevent the application to work without failing compilation.

An example would be something that happened countless times to my students:

package("student_engine", function ()
    add_deps("libsdl")
end)

add_requires("student_engine")
add_requires("libsdl", { configs = { shared = true }})

here we have two versions of libsdl, one static linked to student_engine and one shared linked to the main app, which was causing bugs because libsdl uses global variables internally, they were initializing it in the engine but SDL calls were failing in the app because it wasn't the same libsdl instance.

Another example would be openssl, a common package dependency, which restricts you to use multiple openssl versions across the project, if you use package locking or simply if you set the package version you can get multiple openssl versions.

Describe the solution you'd like

I think xmake should try to merge configs whenever possible, for example:

package("student_engine", function ()
    add_deps("libsdl")
end)

add_requires("student_engine")
add_requires("libsdl", { configs = { shared = true }})

here, student_engine.libsdl doesn't care about being shared or not, so we could merge both configs.

however:

package("student_engine", function ()
    add_deps("libsdl", { configs = { shared = false })
end)

add_requires("student_engine")
add_requires("libsdl", { configs = { shared = true }})

there should be two instances of libsdl here (or an error).

A quick way to do this would be to make add_requires("foo x.y", { configs = { ... }) implicitly add an add_requireconfs("**.foo", { version = "x.y", configs = { ... })` call.

Since it's a breaking change, this could be a policy, but it seems like a good default for xmake 3.0.

Describe alternatives you've considered

Using add_requireconfs by yourself is a way to fix it, however this can lead to more complex xmake.lua and you also need to understand how packages depend on each other, which is not simple.

Simpler xmake should be less error-prone in my opinion.

Additional context

Note that vcpkg does something like this at least for versions: https://learn.microsoft.com/en-us/vcpkg/users/versioning.concepts

@xq114
Copy link
Contributor

xq114 commented Oct 27, 2024

add_requires("libsdl", { configs = { shared = true }, public = true}) Add an argument to indicate passing it to other packages is also ok

@waruqi
Copy link
Member

waruqi commented Oct 27, 2024

add_requires("libsdl", { configs = { shared = true }, public = true}) Add an argument to indicate passing it to other packages is also ok

if public libsdl will modify A and B's libsdl dep configs, foo and bar will both use shared libsdl.

But some users maybe only want to modify foo -> A -> libsdl to shared, bar/B still use static libsdl.

Therefore, this global modification way has too much impact and is not very flexible. it will broke something.

I think the top-level add_requires should be completely independent and independent of each other. Then we can use add_requireconfs to control their dependency configuration more flexibly and finely.

add_requires("A") -- dep: libsdl
add_requires("B") -- dep: libsdl
add_requires("libsdl", { configs = { shared = true }, public = true})

target("foo")
    add_packages("A")

target("bar")
    add_packages("B")

@SirLynix
Copy link
Member Author

I think that xmake default configuration should be the one that lead to minimal library duplication, as it can bloat the app or cause compilation or even runtime bugs (as with libsdl and openssl).

the issue with add_requireconfs is that it requires advance knowledge of how xmake works and to know which libraries depend on each other.

you may want to use openssl 3.2 without being aware that some package you also use will require openssl, leading to duplication (which can even go unnoticed as maybe xmake will find openssl 3.4 on the system).

Therefore, this global modification way has too much impact and is not very flexible. it will broke something.

Yes it may break existing project, which is why I think we should introduce a policy for that and furthermore that this policy should be the default behavior on xmake 3.0

@waruqi
Copy link
Member

waruqi commented Oct 27, 2024

the issue with add_requireconfs is that it requires advance knowledge of how xmake works and to know which libraries depend on each other.

just add_requireconfs("**.libsdl", {configs = {shared = true}}), we need not to know more.

Yes it may break existing project, which is why I think we should introduce a policy for that and furthermore that this policy should be the default behavior on xmake 3.0

I don't think it's a good idea to make this the default behavior for 3.0. It will lead to additional limitations in some package dependency configurations. As I said above.

@SirLynix
Copy link
Member Author

just add_requireconfs("**.libsdl", {configs = {shared = true}}), we need not to know more.

After introducing xmake to my students for four years I can guarantee your that even just configuring libraries in add_requires needs some preparation, add_requireconfs is advanced xmake usage for them.

I don't think it's a good idea to make this the default behavior for 3.0. It will lead to additional limitations in some package dependency configurations. As I said above.

I don't understand why there would be limitations, since add_requireconfs can override each other you can always undo the default behavior using add_requireconfs.

The point of this feature request is to make the default behavior of xmake less error-prone, having add_requires("foo", configs) automatically do an add_requireconfs("**.foo", configs) would be a simple way to fix some of these issues.

@xq114
Copy link
Contributor

xq114 commented Oct 27, 2024

add_requires("libsdl", { configs = { shared = true }, public = true}) Add an argument to indicate passing it to other packages is also ok

if public libsdl will modify A and B's libsdl dep configs, foo and bar will both use shared libsdl.

But some users maybe only want to modify foo -> A -> libsdl to shared, bar/B still use static libsdl.

Why not write like this?

add_requires("libsdl", { configs = { shared = true }, public = true})
add_requires("A") -- dep: libsdl
add_requires("B") -- dep: libsdl
add_requireconfs("B.libsdl", { configs = { shared = false }})

target("foo")
    add_packages("A")

target("bar")
    add_packages("B")

Since B.libsdl is more specific then **.libsdl, the option in B.libsdl should override that in global **.libsdl, just like C++ concept or CSS selector.

@waruqi
Copy link
Member

waruqi commented Oct 28, 2024

When foo/bar only depend on packages A and B, even for beginners, their initial configuration will only think of the following configuration.

add_requires("A") -- dep: libsdl
add_requires("B") -- dep: libsdl

target("foo")
    add_packages("A")

target("bar")
    add_packages("B")

Therefore, they will still encounter libsdl dependency conflicts, or want to modify the configuration of libsdl to use dynamic libraries.

Then they spend the same amount of time figuring out whether to use add_requires or add_requireconfs , or add_requires + public to solve it.

Maybe using add_requires will be slightly faster, but it will bring some global side effects, causing other package dependencies to be modified as well. Users still need to spend more time studying how to use add_requireconfs to modify them again.

@waruqi
Copy link
Member

waruqi commented Oct 28, 2024

add_requires("libsdl", { configs = { shared = true }, public = true}) Add an argument to indicate passing it to other packages is also ok

if public libsdl will modify A and B's libsdl dep configs, foo and bar will both use shared libsdl.
But some users maybe only want to modify foo -> A -> libsdl to shared, bar/B still use static libsdl.

Why not write like this?

add_requires("libsdl", { configs = { shared = true }, public = true})
add_requires("A") -- dep: libsdl
add_requires("B") -- dep: libsdl
add_requireconfs("B.libsdl", { configs = { shared = false }})

target("foo")
    add_packages("A")

target("bar")
    add_packages("B")

Since is more specific then , the option in should override that in global , just like C++ concept or CSS selector.B.libsdl``**.libsdl``B.libsdl``**.libsdl

For the user, the time to read the documentation to solve the problem is the same, but the following configuration is simpler.

add_requires("A") -- dep: libsdl
add_requires("B") -- dep: libsdl
add_requireconfs("A.libsdl", { configs = { shared = true }})

@waruqi
Copy link
Member

waruqi commented Oct 28, 2024

I think a better way is to automatically solve most dependency conflicts without adding any additional configuration. For example, in the same dependency chain, if there are multiple packages with the same name but different configurations, xmake can automatically select the package configuration with the best compatibility.

Automatically select compatible version

This allows us to automatically resolve such issues without requiring the user to add additional add_requires configuration. https://github.com/xmake-io/xmake-repo/pull/5465/files

Maybe we need to improve package configuration to provide more information about compatibility between versions. But I haven't figured out how to do that yet.

Maybe we need to add a new API for packages to describe compatibility information of dependent packages, including (versions, feature configurations, runtimes, etc.)

e.g. A -> libsdl 2.0 -> C -> libsdl 2.8

result: A -> C -> libsdl 2.8

like this

package(A)
    add_deps("libsdl", {compatibility = {version = ">=2.0 <=2.8"}})

If there is no compatibility configuration, all versions are compatible by default.

We can also provide more flexible compatibility strategies based on the current package version.

package(A)
    add_deps("libsdl", {compatibility = function (package)
       if package:version_str() == "1.0" then
            return {version = ">=2.0 <=2.8"}
       end
    end})

Of course, this is not the final API, but just some of my current preliminary design ideas.

Automatically select package configuration compatibility

e.g. A -> boost[filesystem] -> C -> boost[filesystem,asio]

result: A -> C -> boost[filesystem,asio]

Dependency conflicts that cannot be automatically fixed

I hope that automatic repair can solve 80% of dependency problems. In fact, most dependency conflicts are caused by version conflicts.

If the remaining 20% ​​of cases cannot be automatically fixed, we can improve the conflict error prompts and guide users to use add_requireconfs to manually fix them.

@SirLynix
Copy link
Member Author

This seems great! Would the new compatibility syntax be required or would it be automatically taken into account when a package has such deps: add_deps("libsdl >=2.28") or would add_deps("libsdl", {compatibility = {version = ">=2.28"}} be required?

@waruqi
Copy link
Member

waruqi commented Oct 28, 2024

This seems great! Would the new compatibility syntax be required or would it be automatically taken into account when a package has such deps: or would be required?add_deps("libsdl >=2.28")``add_deps("libsdl", {compatibility = {version = ">=2.28"}}

If version compatibility automatic parsing is supported, we can use it directly add_deps("libsdl >=2.28 <=3.0")

@waruqi
Copy link
Member

waruqi commented Oct 29, 2024

But I need to point out that the automatic repair of dependency compatibility can fix the following conflict errors (with version, configs ...)

"package(%s): conflict dependencies with package in ...

But it cannot solve the case mentioned at the beginning of this issue. Because it is not an issue, it is just a usage problem.

As described #5527 (comment), the way package dependencies are handled can cause subtle bugs.
For example
add_requires("libsdl 2.28", "libsdl_ttf")
this will install two different versions of libsdl, because libsdl_ttf doesn't specify a version:
checking for Microsoft Visual Studio (x64) version ... 2022
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
-> libsdl 2.28.5 [runtimes:"MT"]
-> libsdl#1 2.30.8 [runtimes:"MT", from:libsdl_ttf]
-> libsdl_ttf 2.22.0 [runtimes:"MT"]
please input: y (y/n/m)
this may duplicate symbols, fail compilation or even worse, introduce subtle bugs that will just prevent the application to work without failing compilation.

libsdl and libsdl_ttf are both configured at the top level. They are completely independent packages. They will not encounter symbol conflicts when applied to different targets. This is a common usage. Different targets use different versions of sdl

add_requires("libsdl 2.28", "libsdl_ttf")

target("A")
     add_packages("libsdl")

target("B")
    add_packages("libsdl_ttf")

If we only apply them to one target, we should configure it like this instead of adding two add_requires at the same time

add_requires("libsdl_ttf")

target("A")
     add_packages("libsdl_ttf") -- it will also link libsdl

Therefore, I can try to implement automatic repair of package dependency conflicts in 3.0, but I cannot guarantee that the case mentioned in the current issue will be solved because it is not a bug, but just incorrect usage.

@waruqi waruqi added this to the v3.0.0 milestone Oct 29, 2024
@waruqi
Copy link
Member

waruqi commented Nov 29, 2024

now we can resolve version conflicts. #5901

@waruqi
Copy link
Member

waruqi commented Dec 3, 2024

#5923

Now we can solve configs conflict. You can try it.

Merge package configs without conflict automatically

package("foo")
    add_deps("zlib >=1.2.13")
    on_install(function () end)
package_end()

package("bar")
    add_deps("zlib 1.2.x")
    add_deps("boost", {system = false, configs = {lzma = true}})
    on_install(function () end)
package_end()

package("zoo")
    add_deps("boost", {system = false, configs = {zlib = true}})
    on_install(function () end)
package_end()

package("test")
    add_deps("foo", "bar", "zoo")
    on_install(function () end)
package_end()
note: install or modify (m) these packages (pass -y to skip confirm)?
  -> bar latest [from:test]
  -> zoo latest [from:test]
  -> test latest
in xmake-repo:
  -> zlib v1.2.13 [from:foo,bar, license:zlib] (conflict resolved)
  -> boost 1.86.0 [zlib:y, lzma:y, from:bar,zoo, license:BSL-1.0] (conflict resolved)
please input: y (y/n/m)

Unresolvable package configuration conflict

package("bar")
    add_deps("boost", {system = false, configs = {zlib = false}})
    on_install(function () end)
package_end()

package("zoo")
    add_deps("boost", {system = false, configs = {zlib = true}})
    on_install(function () end)
package_end()
package(test): add_deps(boost, ...)
  -> boost 1.86.0 [zlib:n, from:bar, license:BSL-1.0]
  -> boost#1 1.86.0 [zlib:y, from:zoo, license:BSL-1.0]
we can use add_requireconfs("**.boost", {override = true, configs = {}}) to override configs.
error: package(boost): conflict configs dependencies!

@waruqi
Copy link
Member

waruqi commented Dec 3, 2024

We can use set_policy("package.resolve_depconflict", false) to disable resolve deps conflict

@waruqi waruqi modified the milestones: v3.0.0, v2.9.7 Dec 3, 2024
@SirLynix
Copy link
Member Author

SirLynix commented Dec 3, 2024

Amazing! Thanks a lot!

Is it expected to work with configs on project level?

I tested this:

add_rules("mode.debug", "mode.release")
add_requires("libsdl 2.16", "libsdl_ttf")

target("test_pkgconflict")
    set_kind("binary")
    add_files("src/*.cpp")
test_pkgconflict$ xmake f --check
checking for platform ... windows
checking for architecture ... x64
checking for Microsoft Visual Studio (x64) version ... 2022
checking for Microsoft C/C++ Compiler (x64) version ... 19.42.34435
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
  -> libsdl 2.16 [runtimes:"MT", license:zlib]
  -> libsdl#1 2.30.9 [runtimes:"MT", from:libsdl_ttf, license:zlib]
  -> libsdl_ttf 2.22.0 [runtimes:"MT", license:zlib]
please input: y (y/n/m)

@waruqi
Copy link
Member

waruqi commented Dec 3, 2024

Amazing! Thanks a lot!

Is it expected to work with configs on project level?

I tested this:

add_rules("mode.debug", "mode.release")
add_requires("libsdl 2.16", "libsdl_ttf")

target("test_pkgconflict")
    set_kind("binary")
    add_files("src/*.cpp")
test_pkgconflict$ xmake f --check
checking for platform ... windows
checking for architecture ... x64
checking for Microsoft Visual Studio (x64) version ... 2022
checking for Microsoft C/C++ Compiler (x64) version ... 19.42.34435
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
  -> libsdl 2.16 [runtimes:"MT", license:zlib]
  -> libsdl#1 2.30.9 [runtimes:"MT", from:libsdl_ttf, license:zlib]
  -> libsdl_ttf 2.22.0 [runtimes:"MT", license:zlib]
please input: y (y/n/m)

the top-level add_requires should be completely independent, we can only resolve conflicts in the dependency chain.

you still need to use add_requireconfs to modify it.

#5745 (comment)

@SirLynix
Copy link
Member Author

SirLynix commented Dec 3, 2024

Alright.

I tested your example to see if add_requireconfs worked and it does, that neat!

package("foo")
    add_deps("zlib >=1.2.13")
    on_install(function () end)
package_end()

package("bar")
    add_deps("zlib 1.2.x")
    add_deps("boost", {system = false, configs = {lzma = true}})
    on_install(function () end)
package_end()

package("zoo")
    add_deps("boost", {system = false, configs = {zlib = true}})
    on_install(function () end)
package_end()

package("test")
    add_deps("foo", "bar", "zoo")
    on_install(function () end)
package_end()

add_requires("test")
add_requireconfs("**.boost", { configs = { zstd = true }})
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
  -> zlib v1.2.13 [runtimes:"MT", from:bar,foo, license:zlib] (conflict resolved)
  -> boost 1.86.0 [lzma:y, zstd:y, runtimes:"MT", zlib:y, from:zoo,bar, license:BSL-1.0] (conflict resolved)
  -> foo latest [runtimes:"MT", from:test]
  -> bar latest [runtimes:"MT", from:test]
  -> zoo latest [runtimes:"MT", from:test]
  -> test latest [runtimes:"MT"]
please input: y (y/n/m)

if I set a conflicting requireconfs it also properly report it as an error:

add_requires("test")
add_requireconfs("**.boost", { configs = { zlib = false }})
package(test): add_deps(boost, ...)
  -> boost 1.86.0 [lzma:y, runtimes:"MT", zlib:n, from:bar, license:BSL-1.0]
  -> boost#1 1.86.0 [runtimes:"MT", zlib:y, from:zoo, license:BSL-1.0]
we can use add_requireconfs("**.boost", {override = true, configs = {}}) to override configs.
error: package(boost): conflict configs dependencies!

this is really great!

would it be possible to add a policy, as I suggested, which automatically add requireconfs when you set a package config?

set_policy("package.autorequireconf", true)
add_requires("libsdl 2.28", { configs = { sdlmain = false }})

would add a top-level add_requireconfs("libsdl", "**.libsdl", { version = "2.28", { configs = { sdlmain = false }})
as it wouldn't override and be at the top, it would not mess with user requireconfs.

@waruqi
Copy link
Member

waruqi commented Dec 3, 2024

would it be possible to add a policy, as I suggested, which automatically add requireconfs when you set a package config?

I'll consider adding it, but it won't be enabled by default even in 3.0.

@SirLynix
Copy link
Member Author

SirLynix commented Dec 3, 2024

I understand your concerns. What you did is already a great improvement and is good enough for now 😁

@waruqi
Copy link
Member

waruqi commented Dec 4, 2024

you can try package.sync_requires_to_deps.

Although I added it, I personally do not recommend using it, please use add_requireconfs to modify dependency configuration whenever possible.

Using add_requires to synchronize dependencies seems to be much more convenient, but it has many implicit operations.

In addition, add_requires will introduce an additional global unique package instance at the top level. When there are configuration conflicts in multiple different dependency chains, xmake will automatically resolve dependency conflicts, but the configuration modification of this global package instance may have many hidden problems.

zlib >=1.2.11 (toplevel)
test1 -> foo -> zlib -> bar -> zlib (toplevel zlib will be modified if conflict)
test2 -> foo -> zlib -> bar -> zlib (toplevel zlib will be overrided too if conflict)

This can usually only be used for some simple projects with a small number of packages, but for projects with a lot of packages, it is easier to introduce unknown problems.

So this actually increases the complexity of users troubleshooting problems, and it is more concise and clear to use add_requiresconf directly.

because add_requiresconf will not add extra top package instance in toplevel.

package("foo")
    add_deps("zlib >=1.2.13")
    set_policy("package.install_locally", true)
    on_install(function () end)
package_end()

package("bar")
    add_deps("zlib 1.2.x")
    set_policy("package.install_locally", true)
    on_install(function () end)
package_end()

package("zoo")
    set_policy("package.install_locally", true)
    on_install(function () end)
package_end()

package("test")
    add_deps("foo", "bar", "zoo")
    set_policy("package.install_locally", true)
    on_install(function () end)
package_end()

set_policy("package.sync_requires_to_deps", true)

add_requires("test")
add_requires("zlib >=1.2.13", {system = false, configs = {shared = true}})

target("test")
    set_kind("binary")
    add_files("src/*.c")
    add_packages("test")
note: install or modify (m) these packages (pass -y to skip confirm)?
in xmake-repo:
  -> zlib v1.2.13 [shared:y, version:">=1.2.13", from:foo,bar, license:zlib] (conflict resolved
)
  -> bar latest [from:test]
  -> zoo latest [from:test]
  -> test latest
please input: y (y/n/m)

@waruqi waruqi closed this as completed Dec 4, 2024
@SirLynix
Copy link
Member Author

SirLynix commented Dec 4, 2024

Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants