Skip to content

libxml2: resolve missing dependencies when external libraries are needed for optional features #6805

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

Closed
wants to merge 4 commits into from

Conversation

Doekin
Copy link
Contributor

@Doekin Doekin commented Apr 3, 2025

This change enables iconv support by default for libxml2, aligning with its default configuration options.

This PR addresses an issue where enabling iconv can cause build failures for Android due to missing dependencies. The error typically appears as:

ld: error: undefined symbol: iconv
>>> referenced by encoding.c:1122 (/home/runner/.xmake/cache/packages/2504/l/libxml2/v2.14.0/source/encoding.c:1122)
>>>               encoding.c.o:(xmlIconvConvert) in archive /home/runner/.xmake/packages/l/libxml2/v2.14.0/cd35469623984bc18d849f390d29bfb2/lib/libxml2.a

@star-hengxing
Copy link
Contributor

xrepo generally adheres to the principle of minimal dependencies and does not necessarily need to synchronize with upstream options.

@Doekin
Copy link
Contributor Author

Doekin commented Apr 3, 2025

OK, I'll revert the last commit.

@Doekin Doekin changed the title libxml2: enable iconv support by default libxml2: resolve missing dependencies when external libraries are needed for optional features Apr 3, 2025
@luadebug
Copy link
Contributor

luadebug commented Apr 5, 2025

Would it be convenient to define local packagedeps = {} and fill it accordingly to used options table.insert and pass it into (package, configs, {packagedeps = packagedeps})?
Reference https://github.com/xmake-io/xmake-repo/blob/dev/packages%2Fg%2Fgdal%2Fxmake.lua#L118

@Doekin
Copy link
Contributor Author

Doekin commented Apr 5, 2025

Thank you for the suggestion. It should generally be fine to reference non-existing dependencies. If a library is not installed as a dependency, it won't take effect or cause errors during the build process. Therefore, passing all possible dependencies might be a simpler and more concise approach. However, I'm open to further discussion if you think a more selective approach would be better.

@Doekin Doekin marked this pull request as draft April 7, 2025 12:37
@Doekin Doekin marked this pull request as ready for review April 8, 2025 00:50
@@ -142,7 +139,7 @@ package("libxml2")
if lzma and not lzma:config("shared") then
table.insert(cxflags, "-DLZMA_API_STATIC")
end
import("package.tools.cmake").install(package, configs, {cxflags = cxflags, shflags = shflags})
import("package.tools.cmake").install(package, configs, {cxflags = cxflags, shflags = shflags, packagedeps = {"libiconv", "icu4c", "zlib", "xz", "readline"}})
Copy link
Member

@waruqi waruqi Apr 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pass dependencies as needed.

local packagedeps = {}
if package:config("zlib") then
    table.insert(packagedeps, "zlib")
end

In addition, packagedeps are usually not needed, because cmake will automatically find them through add_deps, and xmake will automatically set CMAKE_PREFIX_PATH and PKG_CONFIG_PATH to let cmake/pkg-config find these dependent libraries.

https://github.com/xmake-io/xmake/blob/1bc471e62072071c8cff6b9378d7460a1ce7aa85/xmake/modules/package/tools/cmake.lua#L1017-L1043

Only when cmake really cannot find a library, packagedeps will be used as a hack to support it.
So don't pass all packagedeps at once without any test verification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I'll investigate the root cause of the build failures for Android when enabling iconv, rather than using packagedeps without proper testing. Thank you for the guidance.

@Doekin Doekin closed this Apr 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants