980891 - Build failure on OpenBSD/sparc64 after bug 939276

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Landry Breuil (:gaston)]

m-c fails to build on sparc64 / non-skiagl archs, most probably because of bug 939276:

http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/725/steps/build/logs/stdio

/home/buildslave/mozilla-central-sparc64/build/gfx/thebes/gfxPlatform.cpp: In member function 'virtual void gfxPlatform::InitializeSkiaCacheLimits()':
/home/buildslave/mozilla-central-sparc64/build/gfx/thebes/gfxPlatform.cpp:842:39: error: 'mozilla::hal' has not been declared
       uint32_t totalMemory = mozilla::hal::GetTotalSystemMemory();
                                       ^
/home/buildslave/mozilla-central-sparc64/build/gfx/thebes/gfxPlatform.cpp:856:14: error: invalid use of incomplete type 'class mozilla::gl::SkiaGLGlue'
     mSkiaGlue->GetGrContext()->setTextureCacheLimits(cacheItemLimit, cacheSizeLimit);
              ^
In file included from ../../dist/include/gfx2DGlue.h:9:0,
                 from ../../dist/include/Layers.h:21,
                 from ../../dist/include/mozilla/layers/CompositorParent.h:19,
                 from /home/buildslave/mozilla-central-sparc64/build/gfx/thebes/gfxPlatform.cpp:11:
../../dist/include/gfxPlatform.h:43:7: error: forward declaration of 'class mozilla::gl::SkiaGLGlue'
 class SkiaGLGlue;


probably a missing forward declaration of a class (oh and this is with the patch from https://bugzilla.mozilla.org/show_bug.cgi?id=884376 so SKIA_GPU is undef)

Comment 1

[Steve Singer (:stevensn)]

Attachment: 980891_ifdef_skiagpu.diff

Landry,
Does this patch get things building for you?

Comment 2

[Steve Singer (:stevensn)]

Attachment: 980891_ifdef_skiagpu.diff

Sorry, uploaded the wrong version.

I don't understand why we need to explicitly add the includes to moz.build for non-skia-gpu builds but don't for skia-gpu builds but I seem to require it

Comment 3

[Steve Singer (:stevensn)]

Attachment: 980891_ifdef_skiagpu.diff

Comment 4

[Landry Breuil (:gaston)]

Testing it on next build  (728) of http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64

Comment 5

[Landry Breuil (:gaston)]

Comment on attachment 8388216 [details] [diff] [review]
980891_ifdef_skiagpu.diff

So yes my build was successful with steve's patch..

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 8388216 [details] [diff] [review]
980891_ifdef_skiagpu.diff

Any non-SKIA_GPU platforms should leak as minimally as possible into the codebase.  The lowest code should be fixed to return nulls etc. as appropriate.  Types such as mozilla::gl::SkiaGLGlue and GrContext etc. should be made universally available.  We don't really have any interest in maintaining non-SKIA_GPU code, and this would invariably be broken if #ifdefs are involved.

Better yet, is there any reason why the GPU code doesn't build on your target platform?  Can you just fix it there so that it can be built?

Comment 7

[Steve Singer (:stevensn)]

Attachment: skia_gpu_bendian.diff

As I understand it there are three cases to consider

1) Building without SKIA at all (MOZ_ENABLE_SKIA=0)

This gives the original error reported in this bug.   Parts of my previous patch will probably still be needed for this.

2) Building with MOZ_ENABLE_SKIA=1 but MOZ_ENABLE_SKIA_GPU=0



3) Building on a big endian platform like ppc64 with MOZ_ENABLE_SKIA=1 and MOZ_ENABLE_SKIA_GPU=1

I have attached a patch that accomplishes this, I am not yet sure to what (if any) extent the SKIA or SKIA GL code is working but it compiles.

I am not sure what the best way of handling the defines in the moz.build files are.  ENDIAN order is normally set based on platform in Endian.h. I think I will have to add a similar platform check to configure.in so we can conditional the order of things in the build files.

Comment 8

[Steve Singer (:stevensn)]

Attachment: 980891_noskia.diff

This an updated version of the original patch to get things building when skia is completely disabled.  I no longer change the SkiaGLGlue header.

When SKIA isn't set the skia include files don't get exported so we can't include the SkiaGLGlue header.

Another option might be to somehow change the build files so the skia headers get exported even when skia isn't built.

Comment 9

[Landry Breuil (:gaston)]

Testing att 8396793 in http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/751 (on top of att 8399164 from bug #986366)

Comment 10

[Landry Breuil (:gaston)]

Attachment: 980891_noskia.diff

# sudo -u _buildslave hg qpush                                  
applying attachment.cgi?id=8396793
patching file content/canvas/src/CanvasRenderingContext2D.cpp
Hunk #1 FAILED at 91
Hunk #2 FAILED at 852
2 out of 4 hunks FAILED -- saving rejects to file content/canvas/src/CanvasRenderingContext2D.cpp.rej

Seems this got bitrotted already.. new diff that applies, testing it in
http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/752

Comment 11

[Landry Breuil (:gaston)]

Attachment: 980891_noskia.diff

Doh, that one had a typo misplacing the #endif breaking the build too. That one at least lets gfx build, even if it blows later on in js/ion for something else..

It complains about glue being unused though, maybe i should move it to within the #ifdef ?

content/canvas/src/CanvasRenderingContext2D.cpp:902:21: warning: unused variable 'glue' [-Wunused-variable]
         SkiaGLGlue* glue = gfxPlatform::GetPlatform()->GetSkiaGLGlue();

Comment 12

[Jan Beich]

Aurora needs the same fix but rebased to apply cleanly.

http://buildbot.rhaalovely.net/builders/mozilla-aurora-sparc64/builds/381/steps/build/logs/stdio

cairo-qt defaults to --disable-skia and is also broken.

Comment 13

[Landry Breuil (:gaston)]

Attachment: aurora patch

Same patch backported to aurora

Comment 14

[Gustavo Luiz Duarte]

This affects Linux on ppc64. I was able to build m-c on ppc64 by applying attachment 8399562 [details] [diff] [review] and attachment 8403460 [details] [diff] [review] (from bug 990154).

Comment 15

[Landry Breuil (:gaston)]

vlad, review ping ? Failures are piling on top of each other on exotic archs....

Comment 16

[Andreas Gal :gal]

Note: we intend to obsolete the Cairo backend this year, making skia default everywhere but win32.

Comment 17

[Landry Breuil (:gaston)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/34eaf4712039

(In reply to Andreas Gal :gal from comment #16)
> Note: we intend to obsolete the Cairo backend this year, making skia default
> everywhere but win32.

Great... who will port skia to other archs ? :)

Comment 18

[Andreas Gal :gal]

Patches are always welcome for archs we don't ship on! The pure software path is probably pretty straight forward to add (I assume its mostly fixes to get it to compile).

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/34eaf4712039

Comment 20

[Landry Breuil (:gaston)]

Comment on attachment 8402373 [details] [diff] [review]
aurora patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 939276 
User impact if declined: Failure to build ffx 30 on sparc64
Testing completed (on m-c, etc.): fixes the build
Risk to taking this patch (and alternatives if risky): #ifdef shuffling are always risky...
String or IDL/UUID changes made by this patch: none

Comment 21

[Landry Breuil (:gaston)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/48044e4a4242