Closed Bug 980891 Opened 6 years ago Closed 6 years ago

Build failure on OpenBSD/sparc64 after bug 939276

Categories

(Core :: Graphics, defect)

Sun
OpenBSD
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
Tracking Status
firefox29 --- unaffected
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: gaston, Assigned: stevensn)

References

Details

(Whiteboard: [qa-])

Attachments

(3 files, 5 obsolete files)

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)
Blocks: 939276
Attached patch 980891_ifdef_skiagpu.diff (obsolete) — Splinter Review
Landry,
Does this patch get things building for you?
Attached patch 980891_ifdef_skiagpu.diff (obsolete) — Splinter Review
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
Attachment #8388212 - Attachment is obsolete: true
Attached patch 980891_ifdef_skiagpu.diff (obsolete) — Splinter Review
Attachment #8388214 - Attachment is obsolete: true
Attachment #8388216 - Flags: review?(vladimir)
Comment on attachment 8388216 [details] [diff] [review]
980891_ifdef_skiagpu.diff

So yes my build was successful with steve's patch..
Attachment #8388216 - Flags: feedback+
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?
Attachment #8388216 - Flags: review?(vladimir) → review-
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.
Attached patch 980891_noskia.diff (obsolete) — Splinter Review
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.
Attachment #8388216 - Attachment is obsolete: true
Attachment #8396793 - Flags: review?(vladimir)
Testing att 8396793 in http://buildbot.rhaalovely.net/builders/mozilla-central-sparc64/builds/751 (on top of att 8399164 from bug #986366)
Attached patch 980891_noskia.diff (obsolete) — Splinter Review
# 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
Attachment #8396793 - Attachment is obsolete: true
Attachment #8396793 - Flags: review?(vladimir)
Attachment #8399433 - Flags: review?(vladimir)
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();
Attachment #8399433 - Attachment is obsolete: true
Attachment #8399433 - Flags: review?(vladimir)
Attachment #8399562 - Flags: review?(vladimir)
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.
Attached patch aurora patchSplinter Review
Same patch backported to aurora
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).
vlad, review ping ? Failures are piling on top of each other on exotic archs....
Attachment #8399562 - Flags: review?(vladimir) → review+
Note: we intend to obsolete the Cairo backend this year, making skia default everywhere but win32.
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 ? :)
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).
https://hg.mozilla.org/mozilla-central/rev/34eaf4712039
Assignee: nobody → steve
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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
Attachment #8402373 - Flags: approval-mozilla-aurora?
Attachment #8402373 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.