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

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Remove duplicate SkDiscardableMemory_none.cpp from gfx/skia/moz.build

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.

Comment 2

[Mike Hommey [:glandium]]

Attachment: Always enable skia

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.

Comment 3

[George Wright (:gw280) (needinfo me!)]

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

Comment 4

[Gregory Szorc [:gps]]

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.

Comment 5

[George Wright (:gw280) (needinfo me!)]

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

Comment 6

[Wolfgang Rosenauer [:wolfiR]]

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?

Comment 7

[Mike Hommey [:glandium]]

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

Comment 8

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8a4477dd1516

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8a4477dd1516

Comment 10

[Mike Hommey [:glandium]]

Attachment: Reintroduce pixman code path removed in bug 1097776 for --disable-skia builds

Skia simply doesn't build on non-arm, non-x86, so until it does, it's simpler to just half-backout bug 1097776.

Comment 11

[Landry Breuil (:gaston)]

Confirming that skia doesnt build on sparc64... testing att 8574516

Comment 12

[Landry Breuil (:gaston)]

Related: bug 1005535 tries to enable gpu for skia on ppc, hopefully fixing big endian platforms..

Comment 13

[Landry Breuil (:gaston)]

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

Comment 14

[Cameron Kaiser [:spectre]]

Sounds like --disable-skia wasn't honoured?

Comment 15

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49ccfbcb685a

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49ccfbcb685a

Comment 17

[Martin Stránský [:stransky] (ni? me)]

Attachment: skia big/little endian patch

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

Comment 18

[Cameron Kaiser [:spectre]]

Mike, is this something that could be uplifted to 38 so that it makes the ESR?

Comment 19

[Mike Hommey [:glandium]]

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

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48e130d69836