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

Rust rewrite #99

Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Rust rewrite #99

wants to merge 28 commits into from

Conversation

jim3692
Copy link
Collaborator

@jim3692 jim3692 commented May 7, 2024

This implementation is meant to be a drop-in replacement of the current bash implementation. All Desktop Audio and monitoring are not implemented, as they will probably be temporarily removed in the next release.

It builds without issues with rustc 1.76.0 and cargo 1.76.0, but some dependencies may fail to build on older versions.

Since some distros still distribute older versions of rustc, it is recommended to use rustup in order to get the latest version of rustc and cargo.

To make the installation easier, I would suggest the distribution of static binaries for x64 and aarch64, but we need to see if static linking is possible with the pipewire crate (instead of manually calling pw-cli and pw-dump).

We need to update the flake.nix before merging this

@jim3692 jim3692 self-assigned this May 7, 2024
@jim3692 jim3692 added the enhancement New feature or request label May 7, 2024
@IceDBorn
Copy link
Owner

IceDBorn commented May 7, 2024

Should we include the flake changes into this PR as well?

@jim3692
Copy link
Collaborator Author

jim3692 commented May 7, 2024

Should we include the flake changes into this PR as well?

We should probably do it this way, in order to not break anything (hopefully) while migrating

IceDBorn
IceDBorn previously approved these changes May 8, 2024
Copy link
Owner

@IceDBorn IceDBorn left a comment

Choose a reason for hiding this comment

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

LGTM

@alansartorio
Copy link
Collaborator

Looks good, a few observations:

  1. If you prefer using 2 spaces for indentation we should add a rustfmt.toml in the root of the rust project with the following content:
tab_spaces=2

And then run cargo fmt to fix the formatting

  1. We should be able to remove the unsafe code used in the pw-dump caching, I can help with that
  2. Should we use the debug or the release build? I think the user should be able to cargo clean in the project to free some space without it breaking, but the binary would get deleted. Maybe we want to use cargo install --path . --root ../connector and launch it from there?
  3. There are some eprintlns left behind. We should probably use the log crate exclusively. We can also make it log into a file later, as we cant normally see the stdout.

I recommend you use cargo clippy as the "check command" in rust-analyzer to develop rust, it helps to find improvements.

@IceDBorn
Copy link
Owner

IceDBorn commented May 9, 2024

@alansartorio unspecified tabs please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants