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

libx265: Disable assembly when building for Android #24627

Merged
merged 4 commits into from
Jul 17, 2024

Conversation

uilianries
Copy link
Member

Summary

Changes to recipe: libx265/3.4

Motivation

As reported by the issue #23721 the libx265 fails to be built to Android when using the option value assembly=True. This is not only breaking this package, but also affecting ffmpeg directly when cross-building to Android.

It's a known issue reported in the upstream already, but no official solution so far: https://bitbucket.org/multicoreware/x265_git/issues/603/aarch64-assembly-code-failing-when-cross

Details

To build assembly code in x265, it's used a custom command in the project, which propagates manually which flags should be used by the compiler, resulting in failure when cross-build to Android-armv8.

Other package managers tried custom patches (including Gentoo) to solve the case, but it still fails for me locally. For instance, when passing target to build assembly code:

toolchains/llvm/prebuilt/linux-x86_64/bin/clang++ -fPIC --target=aarch64-none-linux-android32  -c /home/uilian/.conan2/p/b/libx2c4053feaf82aa/b/src/source/common/aarch64/mc-a.S -o mc-a.S.o
<instantiation>:11:9: error: unknown directive
        .func x265_pixel_avg_pp_4x4_neon
        ^
<instantiation>:1:1: note: while in macro instantiation
function x265_pixel_avg_pp_4x4_neon
^
/home/uilian/.conan2/p/b/libx2c4053feaf82aa/b/src/source/common/aarch64/mc-a.S:44:1: note: while in macro instantiation
pixel_avg_pp_4xN_neon 4
^
<instantiation>:2:9: error: unknown directive
        .endfunc
        ^

I'll be disabling assembly for now, but keeping it open for contributions, as didn't remove the option.

Full build log for Android with this new change: https://gist.github.com/uilianries/971fc896b481170ee311ac6e9eb35152

closes #23721

Related to #24566


Signed-off-by: Uilian Ries <uilianries@gmail.com>
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Minor question regarding how the recipe is handling things, otherwise looks good

@@ -49,7 +49,8 @@ def config_options(self):
del self.options.with_numa
# FIXME: Disable assembly by default if host is arm and compiler apple-clang for the moment.
# Indeed, apple-clang is not able to understand some asm instructions of libx265
if self.settings.compiler == "apple-clang" and "arm" in self.settings.arch:
# FIXME: Disable assembly by default if host is Android for the moment. It fails to build
if (self.settings.compiler == "apple-clang" and "arm" in self.settings.arch) or self.settings.os == "Android":
self.options.assembly = False
Copy link
Member

Choose a reason for hiding this comment

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

Is this even working? I've submitted conan-io/conan#16672 to show that changing values in config_options will have no effect if the user is setting the option in the CLI

Copy link
Member Author

Choose a reason for hiding this comment

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

If you change the option value in config_option, it will change the default value only. In case the user pass assembly=True, the validate_build will take care and raise and error. Indeed the user has priority over the value configured via config_options.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the build log shows my command line, I didn't enforce the assembly option to False, but it passes:

https://gist.github.com/uilianries/971fc896b481170ee311ac6e9eb35152

In case passing assembly=True it will fail for sure, the issue #23721 has a very similar build log with my current error.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is how it has always worked since Conan 1.x - setting the value of an option in config_options makes it the "default" value - and the default value is only used if the user did not override it elsewhere (e.g via CLI).

@conan-center-bot

This comment has been minimized.

jcar87
jcar87 previously approved these changes Jul 15, 2024
@AbrilRBS AbrilRBS self-assigned this Jul 15, 2024
AbrilRBS
AbrilRBS previously approved these changes Jul 16, 2024
Copy link
Member

@AbrilRBS AbrilRBS left a comment

Choose a reason for hiding this comment

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

Thanks!

@uilianries
Copy link
Member Author

uilianries commented Jul 16, 2024

Building for Mac M1 it fails too when the option lix265:assembly=True. It's false by default when using Mac M1, but users can override it via profile.

I added a restriction in validate() to avoid any possible error when trying to build it, so users will be aware about.

Kudos @AbrilRBS for spotting it.

@conan-center-bot
Copy link
Collaborator

Conan v1 pipeline ✔️

All green in build 4 (f3187d5b2d351ef5ccb94f86707287d33c54569f):

  • libx265/3.4:
    All packages built successfully! (All logs)

  • libx265/3.2.1:
    All packages built successfully! (All logs)


Conan v2 pipeline ✔️

Note: Conan v2 builds are now mandatory. Please read our discussion about it.

All green in build 4 (f3187d5b2d351ef5ccb94f86707287d33c54569f):

  • libx265/3.4:
    All packages built successfully! (All logs)

  • libx265/3.2.1:
    All packages built successfully! (All logs)

@conan-center-bot conan-center-bot merged commit b81e7e3 into conan-io:master Jul 17, 2024
16 checks passed
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.

[package] libx265/3.4: Failed to build for Android
5 participants