Build OpenVR on macOS and Linux

RESOLVED FIXED in Firefox 56

Status

()

Core
WebVR
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: kip, Assigned: kip)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a month ago
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.
(Assignee)

Updated

a month ago
Assignee: nobody → kgilbert
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8879291 - Flags: review?(dmu)

Comment 4

a month ago
mozreview-review
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?
Attachment #8879291 - Flags: review?(dmu) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a month ago
mozreview-review-reply
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

a month ago
Pushed by kgilbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9303d7ed2b52
Build OpenVR on macOS and Linux r=daoshengmu

Comment 8

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9303d7ed2b52
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
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.

Updated

a month ago
Depends on: 1375693
You need to log in before you can comment on or make changes to this bug.