-
Notifications
You must be signed in to change notification settings - Fork 19
Update to OpenVR SDK 1.0.7 #2
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
Conversation
This also switches to using OpenVR as a submodule.
d6e03e1
to
c840038
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting your changes!
When quickly reading your changes I had a few questions.
I'll try to test the changes tomorrow on my system.
@@ -1,27 +0,0 @@ | |||
Copyright (c) 2015, Valve Corporation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason for removing the license I'm not sure if that's legal. It states:
1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The license was not removed. I removed the existing openvr
subdirectory entirely and replaced it with a submodule pointing to the official repository. This will help keep the size of this repository down and make future updates easier.
exclude = [ | ||
"openvr/samples/*", | ||
"openvr/bin/*", | ||
"openvr/lib/*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excluding the libs might cause issues when someone downloads rust-openvr-sys from cargo I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The binaries in the upstream repository are excluded as of 50e98d5 because the changes include building the OpenVR client code from source and statically linking it. This should make redistributing binaries which use this library much easier, as there's no need to patch runpaths or muck about with dynamic library search paths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, thats great! Thanks
Another change of interest is the use of constified enums. This is a breaking change, but I believe it is necessary because many of the enums defined by OpenVR are open, and reading an unexpected value as a Rust enum value would invoke undefined behavior. |
Closing in favor of #3. |
…nually, workflow for building and testing on all platforms (#15) * Added a workflow to compile on windows, linux, and mac. * Update openvr v2.5.1 (#1) * Update openvr dependency to v2.5.1. * Update dependencies and rebuild bindings - no longer get errors generating bindings or running tests (on Windows at least). * Run actions on every branch. * Remove the buildtime_bindgen feature, lets just always generate bindings since they need to be different on each platform. In the future if needed windows, mac, and linux bindings could be prebuilt and committed and used via a feature for quicker builds. * Fix missing import, add edition to Cargo.toml. * Update version to 2.1.0. * Set min support rust version to 1.82 and test 1.82-1.84 (#2) * Run workflow on multiple versions of rustc. * It seems the unsafe extern issue was fixed in 1.82 so min version will be 1.82 and we will test 1.82 to 1.84. * Remove workaround instructions from README as no longer necessary.
This also switches to using OpenVR as a submodule.