Closed Bug 930256 Opened 11 years ago Closed 10 years ago

Consolidate theora moz.build files

Categories

(Core :: Audio/Video, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: rillian, Assigned: rillian)

Details

Attachments

(1 file, 6 obsolete files)

      No description provided.
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)
Fix problems with building arm assembly.
Attachment #821329 - Attachment is obsolete: true
Attachment #821352 - Flags: review?(ted)
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)
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)
(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)
Attachment #821832 - Attachment is obsolete: true
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)
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+
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
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.