Closed Bug 1310904 Opened 3 years ago Closed 3 years ago

--disable-gamepad build fails: dom/gamepad/ipc/GamepadEventTypes.ipdlh: fatal error: 'mozilla/dom/GamepadMessageUtils.h' file not found

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: jbeich, Assigned: daoshengmu)

References

Details

(Keywords: regression)

Attachments

(4 files, 2 obsolete files)

Gamepad API isn't implemented on many Tier3 platforms: BSDs, Solaris, etc.

In file included from objdir/ipc/ipdl/UnifiedProtocols0.cpp:65:
In file included from objdir/ipc/ipdl/GamepadEventTypes.cpp:7:
objdir/ipc/ipdl/_ipdlheaders/mozilla/dom/GamepadEventTypes.h:20:10: fatal error:
      'mozilla/dom/GamepadMessageUtils.h' file not found
#include "mozilla/dom/GamepadMessageUtils.h"
         ^
1 error generated.

In file included from objdir/ipc/ipdl/UnifiedProtocols13.cpp:29:
In file included from objdir/ipc/ipdl/PGPUChild.cpp:14:
In file included from objdir/ipc/ipdl/_ipdlheaders/mozilla/gfx/PVRManagerChild.h:17:
In file included from objdir/dist/include/VRMessageUtils.h:12:
In file included from objdir/dist/include/VRManager.h:14:
objdir/dist/include/gfxVR.h:18:10: fatal error: 'mozilla/dom/GamepadBinding.h' file not found
#include "mozilla/dom/GamepadBinding.h"
         ^
1 error generated.
Keywords: regression
Assignee: nobody → dmu
I am going to help this.
Jan,
Sorry for inconvenient, could you help me try this patch? This should fix the problem.
Flags: needinfo?(jbeich)
Comment on attachment 8801993 [details] [diff] [review]
Bug 1310904 Hide gamepad header files for tier3 platforms

After applying the patch

  $ ./mach build
  [...]
  In file included from widget/nsBaseWidget.cpp:76:
  In file included from objdir/dist/include/VRManagerChild.h:11:
  In file included from objdir/ipc/ipdl/_ipdlheaders/mozilla/gfx/PVRManagerChild.h:9:
  In file included from objdir/ipc/ipdl/_ipdlheaders/mozilla/gfx/PVRManager.h:20:
  In file included from objdir/ipc/ipdl/_ipdlheaders/mozilla/dom/GamepadEventTypes.h:20:
  objdir/dist/include/mozilla/dom/GamepadMessageUtils.h:6:10: fatal error: 'mozilla/dom/Gamepad.h' file not found
  #include "mozilla/dom/Gamepad.h"
	   ^

  $ find . -name GamepadMessageUtils.h
  ./objdir/dist/include/mozilla/dom/GamepadMessageUtils.h
  ./dom/gamepad/ipc/GamepadMessageUtils.h

  $ find . -name Gamepad.h
  ./dom/gamepad/Gamepad.h

or after moving more headers in dom/gamepad/moz.build outside MOZ_GAMEPAD conditional

  $ ./mach build
  [...]
  widget/nsBaseWidget.cpp:76:
  objdir/dist/include/VRManagerChild.h:11:
  objdir/ipc/ipdl/_ipdlheaders/mozilla/gfx/PVRManagerChild.h:9:
  objdir/ipc/ipdl/_ipdlheaders/mozilla/gfx/PVRManager.h:20:
  objdir/ipc/ipdl/_ipdlheaders/mozilla/dom/GamepadEventTypes.h:20:
  objdir/dist/include/mozilla/dom/GamepadMessageUtils.h:6:
  objdir/dist/include/mozilla/dom/Gamepad.h:11:10: fatal error: 'mozilla/dom/GamepadBinding.h' file not found
  #include "mozilla/dom/GamepadBinding.h"
	   ^

  $ find . -name GamepadBinding.h
  $

Note, adding |ac_add_options --disable-gamepad| to .mozconfig should have the same effect on Tier1 platforms.
Flags: needinfo?(jbeich)
Summary: dom/gamepad/ipc/GamepadEventTypes.ipdlh: fatal error: 'mozilla/dom/GamepadMessageUtils.h' file not found → --disable-gamepad build fails: dom/gamepad/ipc/GamepadEventTypes.ipdlh: fatal error: 'mozilla/dom/GamepadMessageUtils.h' file not found
Adding |ac_add_options --enable-gamepad| to .mozconfig fixes my build. On Tier3 platforms dom/gamepad/fallback/ stub is used.

Ted, why not drop --disable-gamepad support? Is it used by Mozilla somewhere and what's the benefit?
Flags: needinfo?(ted)
It was originally used because platforms that didn't have an implementation would break. The fallback bits are relatively new, but if things compile fine then yes, we could just default to compiling the gamepad code on all platforms. The other function of --disable-gamepad was to allow not using that code because it added extra build-time requirements on some platforms, but that may not be an issue. If it still is, we could make --disable-gamepad simply a switch to use the fallback implementation instead of the default platform implementation.
Flags: needinfo?(ted)
This update should be good.

I notice GampadBinding.h can't be included by any source code that is not belong to Gamepad module. Therefore, I have to find a way to hide GamepadMappingType. That makes my source code a little bit messy and has to do some type casting.

But, I think it already solves our problems. Please think about whether you would like to support MOZ_GAMEPAD mode directly. On the other hand, if you would like to support non MOZ_GAMEPAD mode like before, you can call me to land this patch.
Attachment #8801993 - Attachment is obsolete: true
Comment on attachment 8802116 [details] [diff] [review]
Bug 1310904: Solve compiling errors in non MOZ_GAMEPAD.patch

In file included from objdir/dom/bindings/UnifiedBindings10.cpp:2:
objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:10: error: use of undeclared
      identifier 'ValueToPrimitive'
    if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
         ^
objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:33: error: expected '(' for
      function-style cast or type construction
    if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
                          ~~~~~~^
objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:35: error: use of undeclared
      identifier 'eDefault'
    if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
                                  ^
objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:45: warning: expression result
      unused [-Wunused-value]
    if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
                                            ^~
objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:49: warning: expression result
      unused [-Wunused-value]
    if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
                                                ^~~~~
2 warnings and 3 errors generated.
Attachment #8802116 - Flags: feedback-
(In reply to Jan Beich from comment #7)
> Comment on attachment 8802116 [details] [diff] [review]
> Bug 1310904: Solve compiling errors in non MOZ_GAMEPAD.patch
> 
> In file included from objdir/dom/bindings/UnifiedBindings10.cpp:2:
> objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:10: error: use of
> undeclared
>       identifier 'ValueToPrimitive'
>     if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
>          ^
> objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:33: error:
> expected '(' for
>       function-style cast or type construction
>     if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
>                           ~~~~~~^
> objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:35: error: use of
> undeclared
>       identifier 'eDefault'
>     if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
>                                   ^
> objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:45: warning:
> expression result
>       unused [-Wunused-value]
>     if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
>                                             ^~
> objdir/dom/bindings/KeyframeAnimationOptionsBinding.cpp:67:49: warning:
> expression result
>       unused [-Wunused-value]
>     if (!ValueToPrimitive<double, eDefault>(cx, value, &memberSlot)) {
>                                                 ^~~~~
> 2 warnings and 3 errors generated.

Hi Jan,

This errors seem to be not relative to my patches. Could you help confirm this with clobber build?
Flags: needinfo?(jbeich)
The error is reproducible on FreeBSD 12.0 amd64 as of mozilla-central changeset 90d8afaddf91 with just attachment 8802116 [details] [diff] [review] on top and the following .mozconfig

# -*- sh -*-

mk_add_options AUTOCLOBBER=1

ac_add_options --disable-debug-symbols
ac_add_options --disable-strip
ac_add_options --disable-install-strip
#ac_add_options --enable-debug

ac_add_options --disable-dbus
ac_add_options --disable-gconf

ac_add_options --disable-pulseaudio
ac_add_options --enable-alsa

It's not a new regression as --enable-gamepad builds fine *without* your patch.
Flags: needinfo?(jbeich)
(In reply to Jan Beich from comment #9)
> The error is reproducible on FreeBSD 12.0 amd64 as of mozilla-central
> changeset 90d8afaddf91 with just attachment 8802116 [details] [diff] [review]
> [review] on top and the following .mozconfig
> 
> # -*- sh -*-
> 
> mk_add_options AUTOCLOBBER=1
> 
> ac_add_options --disable-debug-symbols
> ac_add_options --disable-strip
> ac_add_options --disable-install-strip
> #ac_add_options --enable-debug
> 
> ac_add_options --disable-dbus
> ac_add_options --disable-gconf
> 
> ac_add_options --disable-pulseaudio
> ac_add_options --enable-alsa
> 
> It's not a new regression as --enable-gamepad builds fine *without* your
> patch.

Sorry for I can't reproduce your problem.

I have the same config as yours and just mark

#ac_add_options --disable-pulseaudio
#ac_add_options --enable-alsa

because there are no alsa library in my Mac OSX.

Either --disable-gamepad or no --disable-gamepad are good.
See Also: → 1311460
See Also: → 1310862
Comment on attachment 8802116 [details] [diff] [review]
Bug 1310904: Solve compiling errors in non MOZ_GAMEPAD.patch

--disable-gamepad fails on Linux and OS X according to Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e4e7b22918a (implicit --enable-gamepad)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d45ba27443fc (--disable-gamepad)
Update 2:
In this version, I add #ifdef MOZ_GAMEPAD at gfxVR.cpp for gamepad headers.

I have tested it is fine when I apply it after my bug 1299928 patches in Ubuntu 14.04. But if I rebase to the latest Nightly, I will have the same error messages like yours. I believe some patches after mine occur these new regression.

Can you help me confirm it, Jan? Thanks.
Attachment #8802116 - Attachment is obsolete: true
Flags: needinfo?(jbeich)
Comment on attachment 8802864 [details] [diff] [review]
Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)

I confirm. --disable-gamepad build succeeds on FreeBSD with this patch on top of mozilla-central changeset 99eb47ffccb9. However, Try fails, probably due to MOZ_AUTOMATION enabling -Werror by default:

  o on top of mozilla-central changeset 198c4bf0c8df (just before bug 1299928)
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb7c2038edd1 (green)

  o on top of mozilla-central changeset 99eb47ffccb9 (just after bug 1299928)
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0cc979a75c8 (red)

  o on top of mozilla-central changeset 5639a9f476d0 (current m-c tip)
    https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e810f7a54b9 (red)

Continue?
Flags: needinfo?(jbeich)
Attachment #8802864 - Flags: feedback+
Duplicate of this bug: 1310862
Comment on attachment 8802864 [details] [diff] [review]
Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)

stan, can you confirm this patch fixes your build as well?

$ hg up 99eb47ffccb9
$ hg import 0001-Bug-1310904-Solve-compiling-errors-in-non-MOZ_GAMEPA.patch
$ echo ac_add_options --disable-gamepad >> .mozconfig
$ ./mach build
Attachment #8802864 - Flags: feedback?(stanl-mozilla)
See Also: 1310862
(In reply to Jan Beich from comment #14)
> Comment on attachment 8802864 [details] [diff] [review]
> Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)
>
> 
>   o on top of mozilla-central changeset 99eb47ffccb9 (just after bug 1299928)
>     https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0cc979a75c8
> (red)
> 

Jan, thanks for your help. I think these reds are due to enable -werror. [/builds/slave/try-m64-0000000000000000000000/build/src/gfx/vr/gfxVROpenVR.h:122:18: error: private field 'kOpenVRControllerButtons' is not used [-Werror,-Wunused-private-field]
/builds/slave/try-m64-0000000000000000000000/build/src/gfx/vr/gfxVROpenVR.h:123:18: error: private field 'kOpenVRControllerAxes' is not used [-Werror,-Wunused-private-field]
make[6]: *** [Unified_cpp_gfx_vr0.o] Error 1]

They are not a big deal. I have landed some patches in gfxVROpenVR.h, and I promise it would not be red now.

>   o on top of mozilla-central changeset 5639a9f476d0 (current m-c tip)
>     https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e810f7a54b9
> (red)
> 

This should be an another regression. We should continue to use bisect to investigate which commit is the root cause.

> Continue?

We can consider to land attachment 8802864 [details] [diff] [review] and continue to find the root cause of [KeyframeAnimationOptionsBinding.cpp(67): error C2143].

I am working on some high priority bugs. I can continue to investigate this bug in the beginning of November and giving some updates.
OS: FreeBSD → All
Hardware: Unspecified → All
Re: Comment 16

Nightly compiled fine with that patch.  Thanks.
Comment on attachment 8802864 [details] [diff] [review]
Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)

Patch worked ok.  Commented in the bug.
Comment on attachment 8802864 [details] [diff] [review]
Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)

I commented in the main bug, but I keep getting messages about this.  It worked.
I keep getting messages that I need to leave feedback for this bug.  But I don't see anything indicating an action that I need to take.  Can someone clarify?

Overdue requests requiring action from you:
1 feedback

:: FEEDBACK requests

[Bug 1310904] --disable-gamepad build fails: dom/gamepad/ipc/GamepadEventTypes.ipdlh: fatal error: 'mozilla/dom/GamepadMessageUtils.h' file not found
  5 days ago from Jan Beich
  https://bugzilla.mozilla.org/show_bug.cgi?id=1310904
  Review: https://bugzilla.mozilla.org/review?bug=1310904&attachment=8802864
  Defer: https://bugzilla.mozilla.org/request_defer?flag=1475453
attachment 8802864 [details] [diff] [review] has feedback? flag set. To mark it as granted select + in the drop-down menu near near your mail address.
Comment on attachment 8802864 [details] [diff] [review]
Bug 1310904 - Solve compiling errors in non MOZ_GAMEPAD (Update 2)

Thank you Jan.
Attachment #8802864 - Flags: feedback?(stanl-mozilla) → feedback+
Any status updates here? Would be nice if we could this fixed before 52 goes to Aurora in a couple weeks.
Flags: needinfo?(dmu)
Comment on attachment 8807477 [details]
Bug 1310904 - Part 1: Solving non MOZ_GAMEPAD case in GamepadManager;

https://reviewboard.mozilla.org/r/90618/#review90314

LGTM, I wish we can do ifdefs in ipdl files :(
Attachment #8807477 - Flags: review?(cleu) → review+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #24)
> Any status updates here? Would be nice if we could this fixed before 52 goes
> to Aurora in a couple weeks.

I use the newest code from m-c, and I can't reproduce the [KeyframeAnimationOptionsBinding.cpp(67): error C2143] issue that I mentioned at Comment 17. Therefore, I think my current patches could help us fix it.

Let's wait for review, and then we can check-in.
Flags: needinfo?(dmu)
In these patches, I avoid gamepad related source files to be included in non MOZ_GAMEPAD mode. Because we can't do ifdefs in ipdl, the only way we can do is casting them to uint32_t.

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=580379881248
I really don't like the idea of breaking our type system like this. Why can't we only expose the gamepad methods when MOZ_GAMEPAD is defined?
Comment on attachment 8807477 [details]
Bug 1310904 - Part 1: Solving non MOZ_GAMEPAD case in GamepadManager;

https://reviewboard.mozilla.org/r/90618/#review90444
Attachment #8807477 - Flags: review?(kyle) → review+
(In reply to George Wright (:gw280) (:gwright) from comment #32)
> I really don't like the idea of breaking our type system like this. Why
> can't we only expose the gamepad methods when MOZ_GAMEPAD is defined?

Hi George,
That's really I want to do. But the real problem is our IPDL can't use #ifdef, and VRManager is seen by non MOZ_GAMEPAD platforms. (https://dxr.mozilla.org/mozilla-central/rev/8e8b146fcb8b268e3c09b646087c6b2ef9f0af6f/gfx/vr/ipc/PVRManager.ipdl#65)

So, there are some points could discuss:

1. Could we make VR be unavailable in non MOZ_GAMEPAD mode? I love this idea, but it needs to wait for kip comes back to discuss.

2. Do some minor changes like Attachment #8807478 [details] for avoiding to influence our type system as little as possible.

3. Like my attachment 8808045 [details] [diff] [review], undef all gamepad function for non MOZ_GAMEPAD platforms, but it would make our code be messy.

IMHO, I hope we could use the second solution for quick fix and consider adopt the first solution when kip comes back.

How do you think? George.
Flags: needinfo?(gwright)
Personally, it seems to me like 1) is the correct solution here. We have a dependency on gamepad with VR as the code currently stands, so it makes sense to me to make that a formal dependency. 

For now, could we do something like "typedef GamepadMappingType uint32_t" if we're not in a gamepad enabled build, and file a bug to fix it properly when kip gets back? I understand Ryan wants this in before the next train.
Flags: needinfo?(gwright)
(In reply to George Wright (:gw280) (:gwright) from comment #36)
> Personally, it seems to me like 1) is the correct solution here. We have a
> dependency on gamepad with VR as the code currently stands, so it makes
> sense to me to make that a formal dependency. 

Seconding this. These are dependent APIs and so disabling one should disable the other. Would also be nice to have the types cleaner too, though I'm ok with the current solution while the larger one is planned.
See Also: → 1315896
(In reply to George Wright (:gw280) (:gwright) from comment #36)
> Personally, it seems to me like 1) is the correct solution here. We have a
> dependency on gamepad with VR as the code currently stands, so it makes
> sense to me to make that a formal dependency. 
> 
> For now, could we do something like "typedef GamepadMappingType uint32_t" if
> we're not in a gamepad enabled build, and file a bug to fix it properly when
> kip gets back? I understand Ryan wants this in before the next train.

I follow your suggestion to make typedef uint32_t GamepadMappingType, but it doesn't work. It will be ambiguous with the enum class GamepadMappingType of GamepadBinding.h at gfxVROpenVR.cpp.

I have filed Bug 1315896 to trace the progress for removing the dependency between VR and Gamepad. Let's collect some feedback before Kip's back.
Oh, I was expecting the typedef to only be in place for non-MOZ_GAMEPAD builds.
Comment on attachment 8807478 [details]
Bug 1310904 - Part 2: Solving non MOZ_GAMEPAD case in VR;

https://reviewboard.mozilla.org/r/90620/#review91378

r=me providing this will be reverted once bug 1315896 is resolved.
Attachment #8807478 - Flags: review?(gwright) → review+
(In reply to George Wright (:gw280) (:gwright) from comment #40)
> Comment on attachment 8807478 [details]
> Bug 1310904 - Part 2: Solving non MOZ_GAMEPAD case in VR;
> 
> https://reviewboard.mozilla.org/r/90620/#review91378
> 
> r=me providing this will be reverted once bug 1315896 is resolved.

Thanks for your understanding.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/79a80a004aa8
Part 1: Solving non MOZ_GAMEPAD case in GamepadManager; r=lenzak800,qdot
https://hg.mozilla.org/integration/autoland/rev/a348c799d6a6
Part 2: Solving non MOZ_GAMEPAD case in VR; r=gw280
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/79a80a004aa8
https://hg.mozilla.org/mozilla-central/rev/a348c799d6a6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.