Build fails on aarch64-linux in media/libopus

RESOLVED FIXED in Firefox 67

Status

()

defect
P5
normal
Rank:
45
RESOLVED FIXED
3 months ago
2 months ago

People

(Reporter: jseward, Assigned: jbeich)

Tracking

unspecified
mozilla68
ARM64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox66 wontfix, firefox67 fixed, firefox68 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

3 months ago

I know aarch64-linux isn't a primary platform, but still it's useful
to be able to build the browser on it. I'm using Fedora 29 on aarch64
here.

The build fails in media/libopus/silk/arm/arm_silk_map.c, due to this

#include "main_FIX.h"

which, as far as I can see, needs to be

#include "../fixed/main_FIX.h"

instead. Here's the failure message:

0:13.39 /space/MOZ/M_CENTRAL/media/libopus/silk/arm/arm_silk_map.c:31:10: fatal error: main_FIX.h: No such file or directory
0:13.39 31 | #include "main_FIX.h"
0:13.40 | ^~~~~~~~~~~~

(Reporter)

Comment 1

3 months ago
(Reporter)

Comment 2

3 months ago
Comment on attachment 9046677 [details] [diff] [review]
a possible fix

Jean-Marc, does that seem like a plausible fix to you?  It allows
the build to succeed.  I haven't checked whether it causes problems
on any other targets though.
Attachment #9046677 - Flags: feedback?(jmvalin)

Comment 3

3 months ago

The standalone Opus code has no issue compiling on aarch64, but I assume we're talking about a different build system here. The correct solution would be to just have -I/.../libopus/silk/fixed which is what the upstream build does. The change you're proposing would likely work fine, but it would cause unnecessary divergence with the upstream code.

P5 since not a tier-1 build platform.

It looks like the problem might be with the if statement here [1]. Maybe CONFIG['MOZ_SAMPLE_TYPE_FLOAT32'] is set for all linux, but shouldn't be for aarch64 linux?

[1] https://searchfox.org/mozilla-central/source/media/libopus/moz.build#78

Rank: 45
Priority: -- → P5

Comment 5

3 months ago

Well, I guess the first question is which of fixed-point or floating-point was actually intended in the first place. On Aarch64, float is a pretty reasonable option, in which case I don't think silk/arm/arm_silk_map.c should be compiled (I think it's just for fixed-point, but I'd need to check).

Comment on attachment 9046677 [details] [diff] [review]
a possible fix

Closing the feedback? since Jean-Marc responded and this is showing up in triage as an un-answered request.
Attachment #9046677 - Flags: feedback?(jmvalin)
(Assignee)

Updated

2 months ago
Duplicate of this bug: 1536630
(Assignee)

Updated

2 months ago
Blocks: 1522016
(Assignee)

Updated

2 months ago
Keywords: regression
(Assignee)

Comment 8

2 months ago

(Replace f+/f- with r+/r- here! Not on Phabricator as it requires MFA which is incompatible with GitHub login. Too bad, BMO MFA doesn't support 3FA like stacking 2FA of different providers e.g., GitHub 2FA + VideoLAN GitLab 2FA.)

According to standalone build -I./silk/fixed is always passed regardless of whether FIXED_POINT is defined. Let's do the same.
https://github.com/xiph/opus/blob/db082963b983/Makefile.am#L12-L13
http://thunderx1.nyi.freebsd.org/data/head-arm64-default/p495404_s345044/logs/opus-1.3.log
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a8e328fe1843ed909feeb69a187b17d2fdeca628

Attachment #9052144 - Flags: feedback?(core-build-config-reviews)
Blocks: 1532952
Comment on attachment 9052144 [details] [diff] [review]
Add -Isilk/fixed

Review of attachment 9052144 [details] [diff] [review]:
-----------------------------------------------------------------

Please don't abuse splinter.
Attachment #9052144 - Flags: feedback?(core-build-config-reviews) → review+
Assignee: nobody → jbeich

Comment 10

2 months ago

Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40dadf4cbccc
Always pass -Isilk/fixed to unbreak on aarch64 after bug 1522016. r=glandium

Keywords: checkin-needed

Comment 11

2 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Duplicate of this bug: 1539737
(Assignee)

Comment 13

2 months ago

Comment on attachment 9052144 [details] [diff] [review]
Add -Isilk/fixed

Beta/Release Uplift Approval Request

  • Feature/Bug causing the regression: Bug 1522016
  • User impact if declined: Broken build on Linux aarch64, FreeBSD aarch64.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Can only break build or do nothing useful.
  • String changes made/needed:
  • Do you want to request approval of these patches as well?: on
Attachment #9052144 - Flags: approval-mozilla-beta?
Comment on attachment 9052144 [details] [diff] [review]
Add -Isilk/fixed

Build fix for a non supported platform, uplift approved for 67 beta 7, thanks.
Attachment #9052144 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.