1374393 - Build OpenVR on macOS and Linux

Status: RESOLVED FIXED

(Core :: WebVR, enhancement)

Description

[:kip (Kearwood Gilbert)]

OpenVR was previously only being built for Windows builds.  As we will be supporting OpenVR on macOS and Linux, it will need to be included in those builds as well.

Comment 1

[:kip (Kearwood Gilbert)]

Attachment: Bug 1374393 - Build OpenVR on macOS and Linux

- OpenVR was previously only being built for Windows builds
  it will now be built additionally on macOS and Linux.

Review commit: https://reviewboard.mozilla.org/r/150574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/150574/

Comment 2

[:kip (Kearwood Gilbert)]

Comment on attachment 8879291 [details]
Bug 1374393 - Build OpenVR on macOS and Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150574/diff/1-2/

Comment 3

[:kip (Kearwood Gilbert)]

Comment on attachment 8879291 [details]
Bug 1374393 - Build OpenVR on macOS and Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150574/diff/2-3/

Comment 4

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8879291 [details]
Bug 1374393 - Build OpenVR on macOS and Linux

https://reviewboard.mozilla.org/r/150574/#review155988

r=me. Just need to fix MOZ_WIDGET_ANDROID define. Thanks!

::: gfx/vr/VRManager.cpp:23
(Diff revision 3)
>  #include "gfxVROculus.h"
> -#include "gfxVROpenVR.h"
>  #endif
> -#if defined(XP_WIN) || defined(XP_MACOSX) || defined(XP_LINUX)
> +#if defined(XP_WIN) || defined(XP_MACOSX) || (defined(XP_LINUX) && !defined(OS_ANDROID))
> +#include "gfxVROpenVR.h"
>  #include "gfxVROSVR.h"

I am curious if we just need to leave #if !defined(OS_ANDROID), but it is okay for me if you don't want to change.

Besides, it seems to we should use MOZ_WIDGET_ANDROID instead of OS_ANDROID.

MOZ_WIDGET_ANDROID
https://wiki.mozilla.org/Platform/Platform-specific_build_defines

::: gfx/vr/VRManager.cpp:81
(Diff revision 3)
>      mManagers.AppendElement(mgr);
>    }
> -  // OpenVR is cross platform compatible, but supported only on Windows for now
> +#endif
> +
> +#if defined(XP_WIN) || defined(XP_MACOSX) || (defined(XP_LINUX) && !defined(OS_ANDROID))
> +  // OpenVR is cross platform compatible

Replace OS_ANDROID with MOZ_WIDGET_ANDROID would be better.

::: gfx/vr/openvr/README.mozilla:44
(Diff revision 3)
>  - Update the "strtools_public.h" and "strtools_public.cpp" files, commenting out
> -  the "Uint64ToString" function. This function name conflicts with another used
> -  in Gecko. Fortunately, the OpenVR SDK does not use this function either.
> +  the "Uint64ToString", "wcsncpy_s", and "strncpy_s" functions.
> +  The "Uint64ToString" function name conflicts with another used in Gecko and
> +  the "errno_t" return type returned by the other functions is not defined in
> +  Mozilla's macOS continuous integration build environments.  Fortunately, the
> +  OpenVR SDK does not use theese functions.

theese? these?

Comment 5

[:kip (Kearwood Gilbert)]

Comment on attachment 8879291 [details]
Bug 1374393 - Build OpenVR on macOS and Linux

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/150574/diff/3-4/

Comment 6

[:kip (Kearwood Gilbert)]

Comment on attachment 8879291 [details]
Bug 1374393 - Build OpenVR on macOS and Linux

https://reviewboard.mozilla.org/r/150574/#review155988

> I am curious if we just need to leave #if !defined(OS_ANDROID), but it is okay for me if you don't want to change.
> 
> Besides, it seems to we should use MOZ_WIDGET_ANDROID instead of OS_ANDROID.
> 
> MOZ_WIDGET_ANDROID
> https://wiki.mozilla.org/Platform/Platform-specific_build_defines

I've changed OS_ANDROID to MOZ_WIDGET_ANDROID, but left the rest of the conditions so that (however unlikely), we were to add an additional platform, OpenVR wouldn't be automatically included.

> Replace OS_ANDROID with MOZ_WIDGET_ANDROID would be better.

Replaced, thanks for spotting it!

> theese? these?

Thanks for spotting the typo, fixed.. :-)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9303d7ed2b52
Build OpenVR on macOS and Linux r=daoshengmu

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9303d7ed2b52

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I see compiler warnings increasing from this change:
== Change summary for alert #7463 (as of June 22 2017 00:34 UTC) ==

Regressions:

  1%  compiler warnings summary linux32 opt      479.00 -> 486.00
  1%  compiler warnings summary linux64 opt base-toolchains490.00 -> 497.00
  1%  compiler warnings summary linux64 opt valgrind489.00 -> 496.00
  1%  compiler warnings summary linux64-stylo opt 490.00 -> 497.00
  1%  compiler warnings summary linux64 opt      490.00 -> 497.00
  1%  compiler warnings summary linux64-noopt debug 299.00 -> 303.00

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7463

this is more informational instead of asking for a fix as there is no policy for compiler warnings.