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

Linux support, from SlimeVR-OpenVR-Driver #7

Merged
merged 13 commits into from
Feb 22, 2023

Conversation

funnbot
Copy link
Contributor

@funnbot funnbot commented Jan 28, 2023

No description provided.

Copy link
Member

@kitlith kitlith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have the energy to properly review the unix socket code, but that should be okay since it's shared with the driver (and should thus already work and has been tested, barring the issue that butterscotch raised yesterday, which I just fixed)

I'll see if I can clean up the CI stuff myself before merging, so I don't think you'll need to do anything more.

rust_part/CMakeLists.txt Show resolved Hide resolved
.github/workflows/build.yml Show resolved Hide resolved
@kitlith
Copy link
Member

kitlith commented Jan 28, 2023

Shit, CI is failing on linux because of the version embedding code.

Error: /home/runner/work/SlimeVR-Feeder-App/SlimeVR-Feeder-App/src/main.cpp:21:10: fatal error: version.h: No such file or directory

i don't know why. this is kinda important though because I want all the releases to come from CI.

EDIT: I can reproduce on WSL, so it's not limited to CI.

@funnbot
Copy link
Contributor Author

funnbot commented Jan 28, 2023

-- version in git: doesn't actually print the version, so a tag isnt checked out in that case or something?
Happens on windows and ubuntu builds.

@kitlith
Copy link
Member

kitlith commented Jan 28, 2023

-- version in git: doesn't actually print the version, so a tag isnt checked out in that case or something? Happens on windows and ubuntu builds.

that should still result in version.h being generated, just with a blank version. but in my local reproduction (where it is parsing the version correctly, even), version.h just... isn't generated at all. no error or anything, it just isn't there.

@kitlith
Copy link
Member

kitlith commented Jan 28, 2023

-- version in git: is output from the custom command, so it's getting run, but configure_file isn't actually producing anything i guess?

re: your latest commit: i'm ahead of you there, the directories seem to be set correctly

@funnbot
Copy link
Contributor Author

funnbot commented Jan 28, 2023

Windows side prints this warning...

C:\Program Files\Microsoft Visual Studio\2022\Enterprise\MSBuild\Microsoft\VC\v170\Microsoft.CppCommon.targets(247,5): warning MSB8065: Custom build for item "D:\a\SlimeVR-Feeder-App\SlimeVR-Feeder-App\build\CMakeFiles\e9797dbf0013314fb2d386b44164446b\version.h.rule" succeeded, but specified output "d:\a\slimevr-feeder-app\slimevr-feeder-app\build\always_run" has not been created. This may cause incremental build to work incorrectly. [D:\a\SlimeVR-Feeder-App\SlimeVR-Feeder-App\build\SlimeVR-Feeder-App.vcxproj]

OUTPUT "${CMAKE_CURRENT_BINARY_DIR}/version.h" always_run
Ah, so the file is never generated, which ensures it always tries to generate the version.h?

@kitlith
Copy link
Member

kitlith commented Jan 29, 2023

That's the idea, yeah. Was done that way to ensure that locally, a new commit always causes the version to include it on build.

There may be a better way to handle that part of the cmake setup (i.e. it doesn't seem to work on ubuntu, which I know because i tried doing touch build/version.h; chmod 000 build/version.h to at least see if i could get configure_file to error and instead it just skipped it)

@kitlith
Copy link
Member

kitlith commented Jan 29, 2023

okay, if I manually run the custom command, version.h does appear. so why doesn't it appear during the build...?

i grabbed inotifywait and this is what it says happens:

build/ CREATE version.h.tmp
build/ OPEN version.h.tmp
build/ MODIFY version.h.tmp
build/ CLOSE_WRITE,CLOSE version.h.tmp
build/ OPEN version.h.tmp
build/ CREATE version.h
build/ OPEN version.h
build/ CLOSE_NOWRITE,CLOSE version.h.tmp
build/ CLOSE_WRITE,CLOSE version.h
build/ OPEN version.h.tmp
build/ DELETE version.h
build/ CREATE version.h
build/ OPEN version.h
build/ ACCESS version.h.tmp
build/ MODIFY version.h
build/ CLOSE_NOWRITE,CLOSE version.h.tmp
build/ CLOSE_WRITE,CLOSE version.h
build/ ATTRIB version.h
build/ DELETE version.h.tmp
build/ DELETE version.h

so it appearently does get created (at some point), it just gets deleted again afterwards i guess? perhaps it's time to re-do how cmake handles the versioning bit.

@funnbot
Copy link
Contributor Author

funnbot commented Jan 29, 2023

Oh because the makefile generator deletes OUTPUT, I think thats just for clean though...

@kitlith
Copy link
Member

kitlith commented Jan 29, 2023

yeah okay good that fixed that part of CI, but there's a bit of an error in the C++ that only shows up on linux: error: cannot bind non-const lvalue reference of type ‘simdjson::fallback::ondemand::object&’ to an rvalue of type ‘simdjson::fallback::ondemand::object’

@kitlith
Copy link
Member

kitlith commented Jan 29, 2023

@funnbot okay. can you please test this now that it compiles? keep in mind that the fix for #6 isn't in this branch, but if you rebase off of the main branch you should be good.

@ImUrX
Copy link
Member

ImUrX commented Feb 5, 2023

should have pinged me, I need to add the socket in the server lol

@kitlith
Copy link
Member

kitlith commented Feb 6, 2023

@ImUrX ah.

This also has one more thing that needs to be done that I apparently forgot to make a comment on the PR about.

We need to set RPATH so that we actually use the copy of openvr that we bundle with the feeder. In the meantime, setting LD_LIBRARY_PATH at runtime should work as a workaround for testing.

Heh, it probably also needs an update to use XDG_RUNTIME_DIR as well instead of hardcoding /tmp

@ImUrX
Copy link
Member

ImUrX commented Feb 6, 2023

yeah, i mentioned funnbot about that, I can commit it myself tbh

@kitlith
Copy link
Member

kitlith commented Feb 13, 2023

Okay, that should actually be everything, for real this time. Just needs testing (have some artifacts: https://github.com/funnbot/SlimeVR-Feeder-App/suites/10935047338/artifacts/553084572) and then I'll merge it in.

Artifacts for the two platforms are getting merged in CI, I should probably fix that at some point, but it's okay for now, so I'm gonna punt on that. Not a blocker for this PR.

@kitlith
Copy link
Member

kitlith commented Feb 14, 2023

facepalm i've been saying this is finally ready to test, except it isn't, because it still doesn't include the driver rename fix. I'll just do that rebase now.

@ImUrX
Copy link
Member

ImUrX commented Feb 15, 2023

Tested it and it works!

Copy link
Member

@ImUrX ImUrX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nice to also make the action run on PR

@@ -43,10 +43,12 @@ jobs:
- name: CMake+vcpkg configure
run: cmake -S . -B build -DCMAKE_BUILD_TYPE=RelWithDebInfo
shell: cmd
if: runner.os == 'Windows'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change everything to use runner.os or use matrix.os

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

punting to #10

@kitlith kitlith merged commit 7379ba7 into SlimeVR:master Feb 22, 2023
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.

3 participants