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

Revert changes to evdev-rs and uinput crates #51

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

jtroo
Copy link
Owner

@jtroo jtroo commented Jul 21, 2022

This reverts commits:

The reason for doing this is that evdev-rs=0.5.0, evdev-sys=0.2.4 have
a bug where they aren't cached properly and get recompiled on every
build/test/check. This is degrades the development workflow on Linux.

The issue has been fixed but there is no version published to crates.io
that includes the fix.

See: ndesh26/evdev-rs#88

This reverts commits:
- bcbb5ec.
- 2e66b32.

The reason for doing this is that evdev-rs=0.5.0, evdev-sys=0.2.4 have
a bug where they aren't cached properly and get recompiled on every
build/test/check. This is degrades the development workflow on Linux.

The issue has been fixed but there is no version published to crates.io
that includes the fix.

See: ndesh26/evdev-rs#88
@jtroo
Copy link
Owner Author

jtroo commented Jul 21, 2022

@jian-lin due to a bug in the build caching latest version of evdev-rs in crates.io, unfortunately I'll have to revert your recent changes.

These changes can be brought back after ndesh26/evdev-rs#96 is done. Alternatively the evdev crate can be used, though I don't have the bandwidth to make that migration.

@jtroo jtroo merged commit d15fb1b into main Jul 21, 2022
@jtroo jtroo deleted the revert-evdev-uinput-changes branch July 21, 2022 07:02
@jian-lin
Copy link
Contributor

I'm sorry if this bug causes trouble to you.

I agree that a new release of evdev-rs containing the fix is the right way to fix this.

However, there are some possible ways to avoid this revert:

  1. Let evdev-sys use your system's libevdev. This is what I prefer, what I currently use and mentioned by the author of evdev-rs. This is also the reason why I don't encounter this bug. In this case, evdev-sys will not try to build libevdev when you run cargo build or something similar.
  2. Use a git version of evdev-rs containing the fix like this evdev-rs = { git = "https://github.com/ndesh26/evdev-rs.git", rev = "114827702956d3eff80909a87f8ffe175eccbb30" }. The down side of this workaround is that crates.io doesn't allow crates with git dependencies, which will not be a problem if Please publish a new version so that build caching is fixed in crates.io ndesh26/evdev-rs#96 is fixed before our next release.
  3. Only revert the bump commit. Maybe some fixes will be needed for this commit.

What do you think?

@jtroo
Copy link
Owner Author

jtroo commented Jul 21, 2022

I think doing 3. is the best course of action for now. I wouldn't want to do 1. since it may be hard for other potential contributors to find the workaround. And regarding 2., I'd like to be able to publish while I wait for a release.

If you'd like to make a PR to re-introduce bcbb5ec with adjustments to continue compiling, I'll happily accept it. Otherwise I'll get around to it probably around the weekend.

@jian-lin
Copy link
Contributor

jian-lin commented Jul 21, 2022

I wouldn't want to do 1. since it may be hard for other potential contributors to find the workaround

Crates like foo-sys will try to find and use the system's libfoo first, and if that fails, it builds libfoo from scratch. So, IMHO, using system's library is the preferred way.

I am a bit confused why it's hard to do so since all you need to do is to provide pkg-config and libevdev, which I think is already on most users' systems because it's a dependency of libinput.

(edited a typo)

@jian-lin
Copy link
Contributor

I use NixOS and I plan to package kanata for it. Once it is done, contributors are quite easy to setup the development environment.

For mainstream distros like debian, I guess apt install libevdev-dev pkg-config is all you need.

@jian-lin
Copy link
Contributor

jian-lin commented Jul 21, 2022

Also building libevdev from scratch needs autoconf libtool automake python3 pkg-config, while using system's libevdev only needs libevdev-dev pkg-config. IMHO, which one is easier to set up is not that clear to me.

@jtroo
Copy link
Owner Author

jtroo commented Jul 21, 2022

Good questions and useful info. My main issue is not that it's difficult to fix but rather the process of discovering what the fix should be is not straightforward.

To exaggerate, it's like a one-line bug fix that took a week to find.

The problem with this is that the symptom of the issue doesn't immediately stand out but it impacts workflow in a negative way. One of the great things about cargo is that in most cases, it "just works", and doing 1. breaks that. It could be documented in the README, but the documentation might get missed.

So unless there's a very compelling reason to go with 1., I'd rather go with approach 3. until a new release is done for evdev-rs.

Another alternative is:
4. fork evdev-rs like I did with keyberon to be able to release at kanata's desired pace

@jtroo
Copy link
Owner Author

jtroo commented Jul 21, 2022

I should also mention 5. use the evdev instead of evdev-rs crate.

I'll look into that option at a later time as well.

@jian-lin
Copy link
Contributor

One of the great things about cargo is that in most cases, it "just works"

Well, when I try to reproduce your issue, i.e. to let evdev-rs build libevdev from scratch, cargo build "doesn't work" because I don't have autoconf libtool automake in my environment. The error message of failure build of libevdev is quite mysterious if you are new to autotools.

It could be documented in the README, but the documentation might get missed.

IMO, it should be enough.

  1. fork evdev-rs like I did with keyberon to be able to release at kanata's desired pace

Vendering dependencies brings you maintenance burden to keep those dependencies up-to-date, which may cause MANY issues and should only be used as a last resort.

@jtroo
Copy link
Owner Author

jtroo commented Jul 21, 2022

Thanks for the input, it's very helpful. This makes me favour 5. a lot more to avoid the problems of dealing with C code and -sys crates. It'll require a lot more changes from what I can tell, but I think it's worth investigating. I'll be looking into it in the coming days.

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