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

Update Linux PacDrive code (TESTERS NEEDED!!!) #248

Draft
wants to merge 24 commits into
base: beta
Choose a base branch
from

Conversation

sukibaby
Copy link
Contributor

@sukibaby sukibaby commented May 14, 2024

Update the Linux PacDrive code so that it doesn't require legacy libusb.

  • Rewrite the PacDrive code for libusb-1.0
  • Pull libusb-1.0 instead of legacy libusb
  • Test the code on a real PacDrive setup <- We are currently stuck on this step. If you have a PacDrive setup and you can test on Linux, please feel free to help!

Once testing on a real PacDrive has been done, I will undo the libusb-1.0 bodge and properly pull it in as a submodule.

@teejusb
Copy link
Collaborator

teejusb commented May 14, 2024

I think the preference is to bundle libusb as part of extern (as a submodule: https://github.com/libusb/libusb) so that we can build it along with the binary and we don't need to require system libs.

@sukibaby
Copy link
Contributor Author

Should I pull this in? https://github.com/libusb/libusb-cmake

@sukibaby sukibaby marked this pull request as draft May 15, 2024 01:53
@phantom10111
Copy link
Contributor

I'd like to point out that currently itgmania uses legacy libusb-0.1 so modern libusb from github won't work due to API incompatibility. It might be a good idea to upgrade to new libusb first. Or use API wrapper from here: https://github.com/libusb/libusb-compat-0.1

@sukibaby
Copy link
Contributor Author

I'd like to point out that currently itgmania uses legacy libusb-0.1 so modern libusb from github won't work due to API incompatibility. It might be a good idea to upgrade to new libusb first. Or use API wrapper from here: https://github.com/libusb/libusb-compat-0.1

I like the idea of using a compatibility layer because it would be good to avoid rewriting any of the PacDrive stuff, especially since I don't have access to any kind of cabinet. I suppose the CMake build could also be modified so that Linux PacDrive support is one of the optional choices.

@geefr
Copy link
Contributor

geefr commented May 15, 2024

My note on compat is that's what most (all?) linux distros are installing for 0.1 anyway - Can't remember what the changes were but I did have a 3.95 fork compile against libusb 1.0 perfectly fine, think it was mostly just including the right headers and fixing a couple function calls.

Pulling in as a static lib in extern is probably best, unless there's licencing issues (there's not in this case).

Making things optional I'm not sure on for itgmania - They probably should be in cmake, but having everything available by default is nice, even if it makes the binary slightly larger (perhaps as advanced options, so removing features is possible but discouraged?).

(If you're picking this up though suki, thanks - One I can remove from my whiteboard, Happy to advise on cryptic linux things or test when I've got the time)

@teejusb
Copy link
Collaborator

teejusb commented May 15, 2024

I think in order of preference, it would be:

  1. Use libusb 1.0 everywhere and bundle with the binary. This would mean updating the PacDrive code. Testing I think I can send off to Din since he tested it/verified the original PR. I like the idea of standardizing and pulling in updates with submodules is what ITGmania prefers.
  2. Use libusb-compat 0.1. It's lower friction, but I think it's a worse long term option because (1) is better imo.
  3. Include prebuilt binary like in this original PR. Not preferred.

1 >>>> 2 >>>> 3

@sukibaby
Copy link
Contributor Author

I'll change the scope of this PR accordingly:

I think in order of preference, it would be:

  1. Use libusb 1.0 everywhere and bundle with the binary. This would mean updating the PacDrive code. Testing I think I can send off to Din since he tested it/verified the original PR. I like the idea of standardizing and pulling in updates with submodules is what ITGmania prefers.
  2. Use libusb-compat 0.1. It's lower friction, but I think it's a worse long term option because (1) is better imo.
  3. Include prebuilt binary like in this original PR. Not preferred.

1 >>>> 2 >>>> 3

@sukibaby sukibaby changed the title Bundle libusb-0.1.so.4 for Linux x86_64 Update PacDrive code to resolve libusb-0.1 dependency May 16, 2024
@teejusb
Copy link
Collaborator

teejusb commented May 16, 2024

Thank you so much for the work!

@sukibaby
Copy link
Contributor Author

sukibaby commented May 16, 2024

I'll admit I haven't worked with CMake before starting to contribute to ITGMania, so forgive me for being unfamiliar with it, but it seems like it's reporting that libusb can't be found despite the new version being successfully pulled for the makefile.

I couldn't figure out why it was saying this but it seemed to work so i went ahead on other changes for now.

On both my local machine and the Github Ubuntu build test I'm seeing this in the CMake generated Makefile:
.... /usr/lib/x86_64-linux-gnu/libusb-1.0.so ....

and in CMakeCache:

build/CMakeCache.txt:345:LIBUSB_INCLUDE_DIRS:PATH=/usr/include/libusb-1.0
build/CMakeCache.txt:348:LIBUSB_LIBRARIES:FILEPATH=/usr/lib/x86_64-linux-gnu/libusb-1.0.so

however cmake -B build reports:

-- Checking for module 'libusb'
--   No package 'libusb' found

@sukibaby
Copy link
Contributor Author

sukibaby commented May 16, 2024

Here I do not know what I am doing at all:

// i don't know where to put this?
// it shouldn't run every time a device closes
//libusb_exit(NULL);

Attention there would be appreciated very much.

@sukibaby sukibaby marked this pull request as ready for review May 16, 2024 15:02
@sukibaby
Copy link
Contributor Author

sukibaby commented May 17, 2024

I think the preference is to bundle libusb as part of extern (as a submodule: https://github.com/libusb/libusb) so that we can build it along with the binary and we don't need to require system libs.

Is it okay if I leave this up to you? All I did as far as that goes is was change what version of libusb apt pulls in ci.yml and make an ugly tweak in FindLibusb.cmake so I could get going on the new code.

@sukibaby sukibaby marked this pull request as draft May 21, 2024 12:11
@sukibaby sukibaby closed this May 22, 2024
@sukibaby sukibaby deleted the patch-3 branch May 22, 2024 10:54
@sukibaby sukibaby restored the patch-3 branch May 22, 2024 11:02
@sukibaby sukibaby reopened this May 22, 2024
@jsirex
Copy link

jsirex commented May 28, 2024

I have PacDrive at home. If you manage somehow upgrading libusb I can build your branch and test it.

UPD: I've just realized, that I have linux os, so I can build and test this..

@teejusb
Copy link
Collaborator

teejusb commented May 28, 2024

Is it okay if I leave this up to you? All I did as far as that goes is was change what version of libusb apt pulls in ci.yml and make an ugly tweak in FindLibusb.cmake so I could get going on the new code.

Might take some time for me to get around to so we'll see when this will complete.

@teejusb teejusb added the help wanted Extra attention is needed label May 28, 2024
@sukibaby
Copy link
Contributor Author

Is it okay if I leave this up to you? All I did as far as that goes is was change what version of libusb apt pulls in ci.yml and make an ugly tweak in FindLibusb.cmake so I could get going on the new code.

Might take some time for me to get around to so we'll see when this will complete.

Thank you. if it's not done by the time anyone's able to confirm whether the new code works or not, I'll take care of it.

@sukibaby
Copy link
Contributor Author

I have PacDrive at home. If you manage somehow upgrading libusb I can build your branch and test it.

UPD: I've just realized, that I have linux os, so I can build and test this..

Feel free to ping me (here or elsewhere) if you need a hand with this. Thank you!!

@sukibaby
Copy link
Contributor Author

sukibaby commented Jun 5, 2024

I updated the first post of the PR to be a little more helpful.

@sukibaby sukibaby changed the title Update PacDrive code to resolve libusb-0.1 dependency Update Linux PacDrive code (TESTERS NEEDED!!!) Jul 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants