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

Nuke nix #116

Merged
merged 1 commit into from
Feb 21, 2024
Merged

Nuke nix #116

merged 1 commit into from
Feb 21, 2024

Conversation

Jake-Shadle
Copy link
Contributor

I noticed #114 (comment) when I was looking to see why the 0.8.1 version of this crate is using an older version of nix so figured removing nix would be fine. This crate has constantly been using slightly older versions of nix from other dependencies we use, and when I went to replace the use of nix found that the usage was incredibly minimal.

I only tested on linux and had the same test failures as before this change so don't think I broke anything, but I wasn't sure what the intended support for BSD-like targets is since this crate doesn't seem to state anything about them or have CI for them, but I did add support for them, though I failed to figure out to actually get eg. freebsd to build via cross.

@diwic
Copy link
Owner

diwic commented Feb 21, 2024

Hi and thanks a lot for the PR!

I think the whole picture makes sense. A few questions:

  1. Are you the owner of this code or are you copy/pasting it from some other project? Just to make sure there are no accidental license violations here.

  2. I understand this will be a backwards incompatible change, is there a short summary of what a user is supposed to change during the upgrade?

src/error.rs Outdated Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
@Jake-Shadle
Copy link
Contributor Author

  1. No, the code was mostly copied piecemeal from nix, which is MIT licensed so I didn't think it would be problematic.
  2. The only public change is that nix is no longer re-exported, so crates that depend on alsa that just use the re-export instead of their own dependency on nix would need to change, and the methods to retrieve the nix error directly are gone so if a crate was doing a direct conversion from the nix error to their own error type that would need to change.

Copy link
Owner

@diwic diwic left a comment

Choose a reason for hiding this comment

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

Okay, cool. Let's add comments on the bigger things you copy-pasted from nix so we leave a little credit. Then squash all commits into one and I think we're ready for merge.

src/direct/ffi.rs Show resolved Hide resolved
src/error.rs Show resolved Hide resolved
@diwic
Copy link
Owner

diwic commented Feb 21, 2024

...assuming all cargo tests still run fine, that is.

Nuke nix

Undo formatting

Include raw int in error message

Add links to nix src
@diwic diwic merged commit 6dee2c7 into diwic:master Feb 21, 2024
1 check passed
@diwic
Copy link
Owner

diwic commented Feb 21, 2024

...and it's in!

Assuming you want an alsa 0.9 release with this code in it, are there any other dependencies I should update before I go ahead and release a 0.9?

@Jake-Shadle
Copy link
Contributor Author

Thanks! Nope, I don't believe so, libc has been 0.2 for forever and this crate already uses the latest bitflags, it was only nix that was the issue.

@diwic
Copy link
Owner

diwic commented Feb 21, 2024

Released now, enjoy!

@Jake-Shadle
Copy link
Contributor Author

Thanks for the quick turnaround!

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