Consolidate theora moz.build files

RESOLVED FIXED in mozilla36

Status

()

Core
Audio/Video
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: rillian, Assigned: rillian)

Tracking

Trunk
mozilla36
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 6 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 821329 [details] [diff] [review]
0004-Bug-930256-Consolidate-theora-moz.build-files.-r-ted.patch
Attachment #821329 - Flags: review?(ted)
(Assignee)

Comment 2

5 years ago
Comment on attachment 821329 [details] [diff] [review]
0004-Bug-930256-Consolidate-theora-moz.build-files.-r-ted.patch

asm paths aren't correct in this patch.
Attachment #821329 - Flags: review?(ted)
(Assignee)

Comment 3

5 years ago
Created attachment 821352 [details] [diff] [review]
Consolidate theora build files v2

Fix problems with building arm assembly.
Attachment #821329 - Attachment is obsolete: true
Attachment #821352 - Flags: review?(ted)
(Assignee)

Comment 4

5 years ago
Comment on attachment 821352 [details] [diff] [review]
Consolidate theora build files v2

Tim, I'd appreciate your eye on this too, since I repath'd the arm assembly munging.
Attachment #821352 - Flags: review?(tterribe)
Comment on attachment 821352 [details] [diff] [review]
Consolidate theora build files v2

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

I think we should move everything from the Makefile.in to moz.build while you're doing this. No sense in propagating crummy Makefile contents around to a different file.

::: media/libtheora/Makefile.in
@@ +47,5 @@
> +  sse2idct.c \
> +  x86state.c \
> +  x86cpu.c \
> +  $(NULL)
> +endif

Is there a reason we can't move all these CSRCS to moz.build while we're at it? (And ditch the VPATH stuff.)

@@ +82,5 @@
> +armopts-gnu.S: armopts.s
> +	$(PERL) $(srcdir)/lib/arm/arm2gnu.pl < $< > $@
> +# For all others, we can use an implicit rule with the configured $(ASM_SUFFIX).
> +%-gnu.$(ASM_SUFFIX): %.s
> +	$(PERL) $(srcdir)/lib/arm/arm2gnu.pl < $< > $@

I wonder if it wouldn't be saner to write an import script that just did this translation at import-time and committed the result vs. doing this conversion at build-time.
Attachment #821352 - Flags: review?(ted)
(Assignee)

Comment 6

5 years ago
Created attachment 821832 [details] [diff] [review]
Consolidate theora build files v3

Rebase on the list comprehension change from bug 913268.
Attachment #821352 - Attachment is obsolete: true
Attachment #821352 - Flags: review?(tterribe)
Attachment #821832 - Flags: review?(tterribe)
(Assignee)

Comment 7

5 years ago
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)

> I think we should move everything from the Makefile.in to moz.build while
> you're doing this. No sense in propagating crummy Makefile contents around
> to a different file.

Ok. I take it the generator takes the union of the lists in moz.build and Makefile.in?

I'll do an add on patch to move the listings.

> > +	$(PERL) $(srcdir)/lib/arm/arm2gnu.pl < $< > $@
> 
> I wonder if it wouldn't be saner to write an import script that just did
> this translation at import-time and committed the result vs. doing this
> conversion at build-time.

I believe this needs to be done at build time, because the MSVC build uses the intel-syntax files and gcc/clang use the translated ones. Note the -gnu.asm files are conditional on CONFIG['GNU_AS']. I don't think we should have duplicate, generated code in the repo; it's too easy to patch one and not the other.

It also simplifies sharing patches with upstream, but theora hasn't seen any development or bug fixes in a while.
Okay, that sucks. Maybe you can get upstream to switch to YASM. :-P
We also can and have patched the translation script to fix bugs before, e.g., bug 752139.
Comment on attachment 821832 [details] [diff] [review]
Consolidate theora build files v3

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

Unfortunately, this patch has bitrotted (much more appears to have been moved into moz.build, and things are being built in unified mode now). Sorry for taking so long to review. Feel free to re-request review if you rebase this.
Attachment #821832 - Flags: review?(tterribe)
Created attachment 8485933 [details] [diff] [review]
Consolidate theora build files v4
Attachment #821832 - Attachment is obsolete: true
Created attachment 8486005 [details] [diff] [review]
Consolidate theora build files v5

Let's try this again.

Adding LOCAL_INCLUDES over the v4 patch to compensate for no longer exporting theora.h.
Attachment #8485933 - Attachment is obsolete: true
Attachment #8486005 - Flags: review?(tterribe)
Comment on attachment 8486005 [details] [diff] [review]
Consolidate theora build files v5

Adding ted for build peer review.
Attachment #8486005 - Flags: review?(ted)
Attachment #8486005 - Flags: review?(tterribe) → review+
Comment on attachment 8486005 [details] [diff] [review]
Consolidate theora build files v5

Thanks. Android build is busted.
Attachment #8486005 - Flags: review?(ted)
Created attachment 8488279 [details] [diff] [review]
Consolidate theora build files v6

Hoist Makefile.in as well so we can call its rules to generate the asm files. Carrying forward r=derf.
Attachment #8486005 - Attachment is obsolete: true
Comment on attachment 8488279 [details] [diff] [review]
Consolidate theora build files v6

Passing to Ted for build peer review.

https://tbpl.mozilla.org/?tree=Try&rev=4024adf4a02c
Attachment #8488279 - Flags: review?(ted)
Comment on attachment 8488279 [details] [diff] [review]
Consolidate theora build files v6

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

::: media/libtheora/include/moz.build
@@ -3,5 @@
> -# This Source Code Form is subject to the terms of the Mozilla Public
> -# License, v. 2.0. If a copy of the MPL was not distributed with this
> -# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> -
> -DIRS += ['theora']

I don't see a removal of media/libtheora/include/theora/moz.build in this patch.
Attachment #8488279 - Flags: review?(ted) → review+
Created attachment 8524730 [details] [diff] [review]
Consolidate theora build files v7

Remove obsolete libtheora/include/theora/moz.build file, per review comments. Carrying forward r=derf,ted.
Attachment #8488279 - Attachment is obsolete: true
Attachment #8524730 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/59107651d86d
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.