Closed Bug 1136958 Opened 5 years ago Closed 5 years ago

gfx/layers/basic/BasicCompositor.cpp:20:56: fatal error: skia/SkCanvas.h: No such file or directory when skia is disabled

Categories

(Core :: Graphics: Layers, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 --- affected
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: glandium, Assigned: glandium)

References

Details

(Whiteboard: gfx-noted)

Attachments

(3 files, 1 obsolete file)

When building on non-i386, non-x86-64, non-arm, MOZ_ENABLE_SKIA defaults to "". This is equivalent to --disable-skia.

This yields the error in the summary.
Depends on: 1137044
And to do so, cleanup gfx/skia/generate_mozbuild.py a little.

See bug #1137044 for what it takes to use generate_mozbuild.py to generate the same moz.build as what the patch yields.
Assignee: nobody → mh+mozilla
Attachment #8569614 - Flags: review?(gps)
Attached patch Always enable skia (obsolete) — Splinter Review
The code that unconditionally includes skia headers has been added in bug 1097776, and aiui, removed the alternative, making akia mandatory.

So here is one way to deal with the problem: remove the option to do disable skia, considering building skia seems to work on the platforms it is currently disabled on (modulo the duplicate SkDiscardableMemory_none.cpp in gfx/skia/moz.build that is addressed in the other patch).

The alternative would be to half-backout bug 1097776, reintroducing the pixman code path when skia is not enabled.

I don't know which one is better, but at least the former is less involved for me.
Attachment #8569616 - Flags: review?(gwright)
(In reply to Mike Hommey [:glandium] from comment #2)
> So here is one way to deal with the problem: remove the option to do disable
> skia, considering building skia seems to work on the platforms it is
> currently disabled on (modulo the duplicate SkDiscardableMemory_none.cpp in
> gfx/skia/moz.build that is addressed in the other patch).
> 
> The alternative would be to half-backout bug 1097776, reintroducing the
> pixman code path when skia is not enabled.
> 
> I don't know which one is better, but at least the former is less involved
> for me.

I think we need to talk to the gfx team as to whether we are interested in supporting non-skia enabled builds anymore. If not, then let's remove the --disable-skia option entirely. If we do, then let's half-backout bug 1097776.
Flags: needinfo?(milan)
Comment on attachment 8569614 [details] [diff] [review]
Remove duplicate SkDiscardableMemory_none.cpp from gfx/skia/moz.build

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

::: gfx/skia/generate_mozbuild.py
@@ +197,1 @@
>      f.close()

might as well use the context manager version of open while you are here.
Attachment #8569614 - Flags: review?(gps) → review+
Comment on attachment 8569616 [details] [diff] [review]
Always enable skia

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

I'm ok with this if we think dropping --disable-skia is ok (I have no opposition to it personally).
Attachment #8569616 - Flags: review?(gwright) → review+
So I'm not sure I understand here. I get the error as in the summary for some platforms on version 36 but the patch(es) neither do apply to 36 nor do they work there.
At this moment I'm more interested to fix version 36. Any hints?
(In reply to Wolfgang Rosenauer [:wolfiR] from comment #6)
> So I'm not sure I understand here. I get the error as in the summary for
> some platforms on version 36 but the patch(es) neither do apply to 36 nor do
> they work there.

The patch works... on aarch64 (but then the build fails somewhere else)

> At this moment I'm more interested to fix version 36. Any hints?

See comment 2.
Attachment #8569614 - Flags: checkin+
Skia simply doesn't build on non-arm, non-x86, so until it does, it's simpler to just half-backout bug 1097776.
Attachment #8569616 - Attachment is obsolete: true
Attachment #8574516 - Flags: review?(jmuizelaar)
Whiteboard: gfx-noted
Confirming that skia doesnt build on sparc64... testing att 8574516
Related: bug 1005535 tries to enable gpu for skia on ppc, hopefully fixing big endian platforms..
Attachment #8574516 - Flags: review?(jmuizelaar) → review+
With attachment 8574516 [details] [diff] [review], i still get a build failure on OpenBSD/sparc64.

error: #error "SK_*32_SHFIT values must correspond to BGRA or RGBA byte order"

Cf tail of http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/1001/steps/build/logs/stdio
Sounds like --disable-skia wasn't honoured?
https://hg.mozilla.org/mozilla-central/rev/49ccfbcb685a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
(In reply to Landry Breuil (:gaston) from comment #13)
> With attachment 8574516 [details] [diff] [review], i still get a build
> failure on OpenBSD/sparc64.
> 
> error: #error "SK_*32_SHFIT values must correspond to BGRA or RGBA byte
> order"

Try this patch I use for Fedora.
Mike, is this something that could be uplifted to 38 so that it makes the ESR?
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(milan)
Comment on attachment 8574516 [details] [diff] [review]
Reintroduce pixman code path removed in bug 1097776 for --disable-skia builds

Approval Request Comment
[Feature/regressing bug #]: bug 1097776
[User impact if declined]: Can't build Firefox without skia, while skia doesn't support all esoteric platforms.
[Describe test coverage new/current, TreeHerder]: Landed on m-c yesterday, used on Debian for a few weeks.
[Risks and why]: In practice, this patch doesn't change anything for Mozilla builds, and restores the non-skia code path that was removed in bug 1097776.
[String/UUID change made/needed]: None
Attachment #8574516 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1105087
Attachment #8574516 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.