Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Several fixes #16

Merged
merged 14 commits into from
Dec 18, 2022
Merged

Several fixes #16

merged 14 commits into from
Dec 18, 2022

Conversation

rhabacker
Copy link
Contributor

@rhabacker rhabacker commented Nov 22, 2022

The commits from this pr fix problems discovered when building binary packages for the openSUSE distribution, build vsgvr on Linux and Windows MinGW.

They are tested on the Open SUSE build service for example mingw32-vsgvr, mingw64-vsgvr and vsgvr for linux.

They are also needed for further work on #15, since the tests will be based on the use of portable binary packages for Windows built from the above packages.

@rhabacker rhabacker marked this pull request as draft November 22, 2022 10:09
@rhabacker
Copy link
Contributor Author

There is one remaining thing, using openXR from a distribution package.

@rhabacker rhabacker force-pushed the main-fixes branch 2 times, most recently from 88d4ff0 to b5e176f Compare November 22, 2022 12:04
@geefr
Copy link
Owner

geefr commented Nov 22, 2022

I'm not sure things are ready to be added to package managers just yet, but all looks good in principle. There are a lot of changes here, some probably clash with changes I already have, or areas of code that are about to be refactored heavily.

But very much appreciated, will take me a while to integrate - (Ignoring my limited free time) I need to get through a bunch of vsg fixes, then some OpenXR / core features for vsgvr to be actually useful (for a 1.0 at some point). Short version is we're about 80% of the way towards a usable set of functionality, and there's possibly changes to move down to vsg itself at some point.

Some questions for now, otherwise I'll pull commits in when I can:

  • Are you planning many more fixes / changes?
  • Are the suse packages for a specific purpose / to be used by some downstream applications, or just general work to get things building?

@rhabacker
Copy link
Contributor Author

rhabacker commented Nov 23, 2022

Are you planning many more fixes / changes?

Most of the fixes were added to address build issues I encountered when building packages for Linux and Windows (cross-compiled). I do not currently plan to make any major changes to the feature set.

Since I'm not quite done with testing yet, there may still be some minor fixes needed.

By using vsg_setup_xxx macros basic support for install location and
build related settings are available to match other vsg related packages.
To find data files when vsgvr is packaged by a
distribution, the search is extended to the
vsgvr data install location, which is relative
based on the path of the executable and determined
by the cmake buildsystem. This is required to
support a portable installation, such as those
found on Windows.

In order for the executable to find the required
data files when running from the build directory,
the location of the data files in the build
directory directory has been adjusted.
This is required for packaging.
@rhabacker
Copy link
Contributor Author

rhabacker commented Nov 23, 2022

I'm not sure if things are ready to be added to package managers yet,

I use these packages in a personal namespace to get mostly reproducible builds for testing and validation on different platforms and hosts. We are maintaining an osg based VR application and want to see to what extent we could switch to vsg on the supported platforms and what that is missing.

but in principle everything looks good.

Good to hear, I tried to separate the commits by topic.

There are a lot of changes here,

There are three topics:

  1. cmake installation fixes including finding an installed OpenXR package.
    2583d89 Fix static and shared building on Windows
    0b39b86 Use cmake related install settings from vsg
    4fbd0cf Add support to find installed vsgvr data files
    3ab8a26 Add support to install binaries and files
    a3d8f68 Install versioned vsgvr library (packaging requirement)
    421cdc3 Add support to use OpenXR sdk from distribution
    07b51cb Fix finding include headers

  2. toolchain related source code build fixes
    e4a3b77 Fix possible crash in OpenXRActionPoseBinding::destroyActionSpace()
    081f2a3 Fix build error with gcc 7.5: 'cerr|cout' is not a member of 'std'.
    6395742 Fix build error: cannot convert 'std::nullptr_t' to 'XrSpace' {aka 'long long unsigned int'} in assignment

  3. other nice to have commits
    5285688 Cleanup displaying errors and exit code
    185e4cb Use additional cmake related macros from vsg
    0a0c103 Use vsg provided macro vsg_add_feature_summary()
    8c3ede1 Create and install executable as `vsgvrviewer' in order to use a consistent name

Topic 1. and 2. are needed to e.g. build a package on windows

some probably clash with changes I already have, or areas of the code that will be heavily refactored.

Which commits are you referring to?

In case the vsgvr is build as shared library
it needs to be versioned, which is a requirement
for example on openSUSE distributions.
This is required for packaging.
In case _space is 0, the application will crash.
…ong long unsigned int'} in assignment

This happened with mingw gcc12.
This adds cmake target like 'cppcheck' for static code analysis,
'clang-format' for code formatting, 'clobber' to clean git sources,
'docs' for generating api documentation.
…a consistent name

The target name is currently not changed to avoid collisions with further
restructuring.
@geefr
Copy link
Owner

geefr commented Nov 30, 2022

(No need to worry about the conflicts I'm causing, I'll probably have a fair few more, then think about the cmake changes/etc)

@geefr geefr merged commit 4b7a68a into geefr:main Dec 18, 2022
@geefr
Copy link
Owner

geefr commented Dec 18, 2022

@rhabacker Okay almost all of this was merged without issue. I'm reasonably confident it hasn't broken things too badly, but have only checked the windows build so far. I'll check Android shortly, Linux when I have time.

The CMake bits for cppcheck/clangformat/doxygen/clobber are turned off for now, they weren't working right with visual studio, and there's a lot to fix before thinking about code style :)

Thanks for the fixes - If anything's broken or there's other tweaks, best to raise an issue before the PR. Upcoming plans involve trying to use vsgvr for some more demos, which I'm sure will mean I need to go back and rewrite some things extensively..

@rhabacker rhabacker deleted the main-fixes branch February 20, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants