Closed Bug 1530659 Opened 2 years ago Closed 2 years ago

Build fails on aarch64-linux in media/libopus


(Core :: Audio/Video, defect, P5)




Tracking Status
firefox-esr60 --- unaffected
firefox66 --- wontfix
firefox67 --- fixed
firefox68 --- fixed


(Reporter: jseward, Assigned: jbeich)




(2 files)

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

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 | ^~~~~~~~~~~~

Attached patch a possible fixSplinter Review
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)

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?


Rank: 45
Priority: -- → P5

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)
Duplicate of this bug: 1536630
Blocks: 1522016
Keywords: regression
Attached patch Add -Isilk/fixedSplinter Review

(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.

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

Pushed by
Always pass -Isilk/fixed to unbreak on aarch64 after bug 1522016. r=glandium

Keywords: checkin-needed
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
Duplicate of this bug: 1539737

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.