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

media-libs/x265: Fix build on x32 by disabling assem… #173

Merged
merged 1 commit into from Oct 14, 2015

Conversation

bjacquin
Copy link
Contributor

@bjacquin bjacquin commented Oct 9, 2015

…bly code, patch to disable automagic march setup, bug #510890

Package-Manager: portage-2.2.20.1

@bjacquin bjacquin changed the title media-libs/x265: media-libs/x265: Fix build on x32 by disabling assem… media-libs/x265: Fix build on x32 by disabling assem… Oct 9, 2015
@mgorny
Copy link
Member

mgorny commented Oct 10, 2015

Commit message is too long. Put very short explanation on summary line, then explain the whole thing in detail in body, i.e.:

media-libs/x265: Fix x32 build

Fix build on x32 ...

@mgorny
Copy link
Member

mgorny commented Oct 10, 2015

@gentoo/video

@mgorny mgorny added bugfix assigned PR successfully assigned to the package maintainer(s). labels Oct 10, 2015
@bjacquin
Copy link
Contributor Author

Pull requested updated as requested, upstream report made in videolan/x265#1

@mgorny
Copy link
Member

mgorny commented Oct 10, 2015

Errr, sorry for being picky, but I think pasting the build log in commit message is not a good idea ;-). Word the minimal description someone may need to understand the change. In particular, the description you gave on upstream commit is better than the one here ;-).

@bjacquin
Copy link
Contributor Author

@bjacquin
Copy link
Contributor Author

Can you tell me if this is better now ?

@mgorny
Copy link
Member

mgorny commented Oct 11, 2015

Yes, looks good. Just to be clear, what happens when you enable assembly on x32? Does it fail? I suggest you file a bug upstream about that too, they may want to update their asm, or implicitly disable it when unsupported ABI is used.

@bjacquin
Copy link
Contributor Author

@bjacquin
Copy link
Contributor Author

PR updaded for x265 1.8

@aballier
Copy link
Contributor

lgtm, thanks!

just one nitpick: it'd be good to have the bug number in a comment close to where you disable asm, as in my original suggestion: Unless asm optimizations are never intended to work on x32, I imagine a few years from now, people will wonder why this is disabled without warning :)

@@ -54,6 +58,8 @@ multilib_src_configure() {
if [ "${ABI}" = x86 ] ; then
use 10bit && ewarn "Disabling 10bit support on x86 as it does not build (or requires to disable assembly optimizations)"
mycmakeargs+=( -DHIGH_BIT_DEPTH=OFF )
elif [ "${ABI}" = x32 ] ; then
Copy link
Member

Choose a reason for hiding this comment

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

If I may add my own nitpick, please use bash [[ ... ]]. Then quoting becomes unnecessary:

elif [[ ${ABI} = x32 ]] ; then

If that wouldn't be a problem, please also change the other conditional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR updated as per your comment

x32 arch as defined on https://sites.google.com/site/x32abi is neither
X86 nor X64, then forcing -march=i686 leads to build failure as wrong
-march is used.

Forcing -march, -mfloat-abi and -mfpu for ARM is also wrong
As a global sanity sake, disable all forced -march in CMakeLists

Upstream report: https://bitbucket.org/multicoreware/x265/pull-requests/21/build-disable-march-selection-from/diff

Package-Manager: portage-2.2.20.1
@bjacquin
Copy link
Contributor Author

PR updated as per your comment

@aballier
Copy link
Contributor

if you wish to update per above nitpicks, feel free, otherwise it's good to go

and I now see why you were using MULTILIB_ABI_FLAG: it is used in src_test...

@gentoo-bot gentoo-bot merged commit 5658f1d into gentoo:master Oct 14, 2015
mgorny added a commit that referenced this pull request Oct 14, 2015
@mgorny
Copy link
Member

mgorny commented Oct 14, 2015

@bjacquin, your PGP key still seems to be missing on sks keyservers ;-). Please upload it. (gpg --keyserver keys.gnupg.net --send-key YOUR-KEY-ID)

@bjacquin
Copy link
Contributor Author

Hi Michał,

I'm using a EdDSA GnuPG key and therefore keys.gnupg.net does not
support it, but the key is available here:
https://sks-keyservers.net/pks/lookup?op=get&search=0xA3B5C016618D9AAA

Cheers,
Bertrand

On 14/10/2015 22:10, Michał Górny wrote:

@bjacquin [1], your PGP key still seems to be missing on sks
keyservers ;-). Please upload it. (gpg --keyserver keys.gnupg.net
--send-key YOUR-KEY-ID)

Reply to this email directly or view it on GitHub [2].

Links:

[1] https://github.com/bjacquin
[2] #173 (comment)

Bertrand

@bjacquin bjacquin deleted the dev/beber/x32-x265 branch January 11, 2017 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s).
Projects
None yet
4 participants