1876554 - duplicate entries in moz.build files break the build on OpenBSD/riscv64

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Landry Breuil (:gaston)]

Attachment: openbsd-riscv64.diff

an OpenBSD developer is working on enabling the build of firefox on riscv64, but so far the build fails because of duplicate sections in some webrtc moz.build generated files:

mozbuild.frontend.reader.SandboxValidationError:
==============================
FATAL ERROR PROCESSING MOZBUILD FILE
==============================
The error occurred while processing the following file or one of the files it includes:
/local/pobj/firefox-121.0.1/firefox-121.0.1/third_party/libwebrtc/common_audio/third_party/spl_sqrt_floor/spl_sqrt_floor_gn/moz.build
The error occurred when validating the result of the execution. The reported error is:
/third_party/libwebrtc/common_audio/third_party/spl_sqrt_floor/spl_sqrt_floor.c from UNIFIED_SOURCES would have the same object name as 
/third_party/libwebrtc/common_audio/third_party/spl_sqrt_floor/spl_sqrt_floor.c from UNIFIED_SOURCES in non-unified builds.

there are apparently multiple sections like this, i'm attaching a patch against 122.0 to list them, but since iirc those files are autogenerate from some contraption, i suppose the proper fix is ... 'somewhere else'. and since libwebrtc might have been upgraded in the meantime...

Comment 1

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Michael, you may be interested in this.

Comment 2

[Michael Froman [:mjf]]

Generating the moz.build files under libwebrtc is done by running:

./mach python python/mozbuild/mozbuild/gn_processor.py dom/media/webrtc/third_party_build/gn-configs/webrtc.json

The fix is probably some combination of modifying either python/mozbuild/mozbuild/gn_processor.py and/or third_party/libwebrtc/common_audio/third_party/spl_sqrt_floor/BUILD.gn to write a different moz.build file. :glandium may have further info to add.

Comment 3

[Mike Hommey [:glandium]]

This is essentially another instance of bug 1775202. The script is dumb-ish, and that shows. spl_sqrt_floor.c is behind the following conditions:

if CONFIG["OS_TARGET"] == "Darwin":
if CONFIG["OS_TARGET"] == "OpenBSD":
if CONFIG["OS_TARGET"] == "WINNT":
if CONFIG["TARGET_CPU"] == "arm":
if CONFIG["TARGET_CPU"] == "mips32":
if CONFIG["TARGET_CPU"] == "mips64":
if CONFIG["TARGET_CPU"] == "ppc64":
if CONFIG["TARGET_CPU"] == "riscv64":
if CONFIG["OS_TARGET"] == "Android" and CONFIG["TARGET_CPU"] == "aarch64":
if CONFIG["OS_TARGET"] == "Android" and CONFIG["TARGET_CPU"] == "x86":
if CONFIG["OS_TARGET"] == "Android" and CONFIG["TARGET_CPU"] == "x86_64":
if CONFIG["OS_TARGET"] == "Linux" and CONFIG["TARGET_CPU"] == "aarch64":
if CONFIG["OS_TARGET"] == "Linux" and CONFIG["TARGET_CPU"] == "x86":
if CONFIG["OS_TARGET"] == "Linux" and CONFIG["TARGET_CPU"] == "x86_64":

Of course, since they aren't all mutually exclusive, that doesn't work...

Comment 4

[Mike Hommey [:glandium]]

One way to help would be to make target_cpus for openbsd contain all the CPUs it supports.
https://searchfox.org/mozilla-central/rev/2a867dd1ab015c3ef24b774a57709fb3b3dc4961/python/mozbuild/mozbuild/gn_processor.py#735-738

Comment 5

[Mike Hommey [:glandium]]

In ways, it's similar to bug 1760484.

Comment 6

[Mike Hommey [:glandium]]

But of course, gn doesn't work with the combination openbsd+riscv64, barfing:

ERROR Unresolved dependencies.
//:dcsctp(//build/toolchain/linux:clang_riscv64)
  needs //build/toolchain/linux:clang_riscv64()
//:poison_audio_codecs(//build/toolchain/linux:clang_riscv64)
  needs //build/toolchain/linux:clang_riscv64()

etc.

Comment 7

[Landry Breuil (:gaston)]

jca@openbsd has access to riscv64 and is working on a proper fix in gn_processor.py. It also needs some work in xpcom but that's unrelated to libwebrtc.

my understanding is that:

• gn_processor needs the correct arch list (first patch)
• and with that, one needs to run it (on linux ? on OpenBSD ?) to generate the moz.build changes (second patch)
• and one needs to test the resulting moz.build on OpenBSD/riscv64 to confirm it builds
  right ?

Comment 8

[Jeremie Courreges-Anglas [:jca]]

(In reply to Mike Hommey [:glandium] from comment #6)

But of course, gn doesn't work with the combination openbsd+riscv64, barfing:

ERROR Unresolved dependencies.
//:dcsctp(//build/toolchain/linux:clang_riscv64)
  needs //build/toolchain/linux:clang_riscv64()
//:poison_audio_codecs(//build/toolchain/linux:clang_riscv64)
  needs //build/toolchain/linux:clang_riscv64()

etc.

jca@openbsd here. I'm stuck on the same issue, trying to regen the moz.build files after patching gn_processor.py. Are you attempting this from an OpenBSD machine?

(To get to that error on an OpenBSD machine, I had to drop "android" from target_os, because of the assert at third_party/libwebrtc/build/config/BUILDCONFIG.gn:222).

Comment 9

[Jeremie Courreges-Anglas [:jca]]

Attachment: libwebrtc-openbsd-riscv64.diff

OK so I managed to regen the libwebrtc moz.build files using some hacks.

1. The third_party/libwebrtc/build/toolchain/linux/BUILD.gn diff adds support for native (not cross-compile) clang_riscv64 on Linux. The change was copied from the other clang_$arch blocks in the same file.

2. The python/mozbuild/mozbuild/gn_processor.py diff adds support for OpenBSD/riscv64.

3. The third_party/libwebrtc/build/config/BUILDCONFIG.gn and third_party/libwebrtc/build/config/android/config.gni diffs are only here to let me run gn for the Android target from OpenBSD. Else the resulting diff for the moz.build files is larger and less clear.

4. The diff for the various moz.build files shows that the riscv64-specific lines that resulted in duplicated additions to UNIFIED_SOURCES are now guarded with:

-if CONFIG["TARGET_CPU"] == "riscv64":
+if CONFIG["OS_TARGET"] == "Linux" and CONFIG["TARGET_CPU"] == "riscv64":

... which appears to be the desired outcome, and would avoid the need for libwebrtc/*/moz.build patches in the OpenBSD port. IIUC we'd need to properly submit 1. and 2.

Disclaimer: I'm not sure why Linux appears to be handled differently from the other platforms here.

Comment 10

[Nico Grunbaum [:ng, @chew:mozilla.org]]

To ensure that this works smoothly with the libwebrtc update process. I think we need to turn this into 3 patches before we land it. The first with the gn_processor.py changes, the second will have the changes to the GN files under third_party/libwebrtc, and the third will have only the generated moz.build files. Either :mjf or I can handle that after review.

:glandium, will this require uplift?

Comment 11

[Mike Hommey [:glandium]]

That would be a question for the openbsd people.

Comment 12

[Jeremie Courreges-Anglas [:jca]]

(In reply to Mike Hommey [:glandium] from comment #11)

That would be a question for the openbsd people.

IIUC an uplift is a request for changes to be integrated in the next firefox releases. If so, yes I would appreciate that, as I'd like to save :gaston some trouble with conflicting patches at update time (for example the update from 121 to 122 lead to conflicts in the patched moz.build files).

Comment 13

[Jeremie Courreges-Anglas [:jca]]

(In reply to Nico Grunbaum [:ng, @chew:mozilla.org] from comment #10)

To ensure that this works smoothly with the libwebrtc update process. I think we need to turn this into 3 patches before we land it. The first with the gn_processor.py changes, the second will have the changes to the GN files under third_party/libwebrtc, and the third will have only the generated moz.build files.

I have two questions:

• should the third_party/libwebrtc/build/toolchain/linux/BUILD.gn patch happen first, before the patch for gn_processor.py? Without the former, Mike and I get an error when regenerating the moz.build files.
• regarding "the changes to the GN files under third_party/libwebrtc", I think only the patch for third_party/libwebrtc/build/toolchain/linux/BUILD.gn is valid to commit. The others are hacks to let me run gn and regen the moz.build files on OpenBSD but I have no idea if they are correct or meaningful.

Either :mjf or I can handle that after review.
For a review to happen, is the diff I posted earlier good enough? Or shall I clean up it up and split it into three commits?
Is this bug report enough for the review, or should I learn to use Phabricator...?

Sorry for the rookie questions.

Comment 14

[Michael Froman [:mjf]]

Attachment: Bug 1876554 - libwebrtc change to support openbsd riscv64 toolchain. r?ng!

Comment 15

[Michael Froman [:mjf]]

Attachment: Bug 1876554 - update gn_processor.py to support openbsd on riscv64. r?ng!

Comment 16

[Michael Froman [:mjf]]

Attachment: Bug 1876554 - libwebrtc changes to support openbsd on riscv64 - moz.build file updates. r?ng!

Comment 17

[Michael Froman [:mjf]]

try builds: https://treeherder.mozilla.org/jobs?repo=try&revision=22574a6a7c769f0d69bd0460bd947616d71514f9

Comment 18

[Michael Froman [:mjf]]

Comment on attachment 9378510 [details] [diff] [review]
libwebrtc-openbsd-riscv64.diff

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

These changes were good, but all in a combined patch.  I've submitted a broken up patch set and a try run.

Comment 19

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Sorry for the rookie questions.

No worries at all. Thanks for submitting the patches. As for Phabricator, it would speed things up for us if future patches were submitted that way, and it would be appreciated, but do not let that become an impediment.

Comment 20

[Michael Froman [:mjf]]

Try builds look good with the exception of 'build-linux64-plain-clang-trunk/opt', but it fails with out our changes (see here).

Comment 21

[Landry Breuil (:gaston)]

i haven't followed closely, but the phabricator reviews match what :jca sent me that produced a working binary on OpenBSD/riscv64 for him, so i'm all for it :)

as for the uplifting, if webrtc hasnt been updated in the 123/124 timeframe and its not a hassle, i'll welcome an uplift to beta (while it might be late for 123 ?) otherwise the commits will probably be shipped as local patches until 124 is released.

Comment 22

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This needs a rebase for a conflict with the CLOBBER file.

Comment 23

[Michael Froman [:mjf]]

Yes, agreed - I'm waiting on another patch to land before I can rebase. Thank you for the heads up.

Comment 24

[Mike Hommey [:glandium]]

This doesn't need to touch CLOBBER at all, though.

Comment 25

[Michael Froman [:mjf]]

(In reply to Mike Hommey [:glandium] from comment #24)

This doesn't need to touch CLOBBER at all, though.

The libwebrtc update scripts touch the CLOBBER file anytime we generate moz.build files and commit the changes. In the past, we've not been able to programmatically determine when this is needed and when it is not. If you can describe an algorithm that allows us to make that determination and that does not hide build failures do to stale object files left over from previous builds, I'd definitely like to add that functionality to our scripts.

I'm also waiting for a change from 1879898 because we're missing a build file change that was missed.

Comment 26

[Mike Hommey [:glandium]]

Generally speaking, there shouldn't be a need to CLOBBER unless a source file is renamed from e.g. .c to .cpp with the same base name. There might be other corner cases, but that's the main one.

Comment 27

[Michael Froman [:mjf]]

We've run into the issue enough on libwebrtc updates that I'm concerned we exercise too many of those corner cases during libwebrtc updates to safely ignore them.

Comment 28

[Mike Hommey [:glandium]]

Well, in this specific case, there's no concern.

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9170e1167dfe
libwebrtc change to support openbsd riscv64 toolchain. r=ng,webrtc-reviewers
https://hg.mozilla.org/integration/autoland/rev/d67778b576e8
update gn_processor.py to support openbsd on riscv64. r=ng
https://hg.mozilla.org/integration/autoland/rev/3b6d2e1cea3c
libwebrtc changes to support openbsd on riscv64 - moz.build file updates. r=ng,webrtc-reviewers

Comment 30

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/9170e1167dfe
https://hg.mozilla.org/mozilla-central/rev/d67778b576e8
https://hg.mozilla.org/mozilla-central/rev/3b6d2e1cea3c