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

add derive(Debug) for InputEvent #2

Merged
merged 1 commit into from
May 29, 2018

Conversation

soenkehahn
Copy link
Contributor

This PR adds #[derive(Debug)] attributes to InputEvent and to all types that could be contained in an InputEvent. Is there a reason why this is not wanted?

Thanks for the nice library!

@ndesh26
Copy link
Owner

ndesh26 commented May 28, 2018

I am really happy that people are using my library. It makes me even happier that people are contributing to it.

There's no real reason to not have the Debug trait. It's just that I never really needed it. But its good to have it. But the patch is incorrect because the src/enums.rs file should not be modified directly but should be generated using tools/make-event-names.py. I have not documented this part anywhere so that's my fault.

For this patch please update the make-event-names.py (the functions print_enums and print_event_code) and send the patch as is. I was planning to bump libevdev soon, I'll regenerate the src/enums.rs file then.

@soenkehahn
Copy link
Contributor Author

@ndesh26: Thanks for your reply. I'm looking into tools/make-event-names.py right now. I think it is fairly self-explanatory so far, but it doesn't work: I tried executing ./tools/make-event-names.py /usr/src/linux-headers-4.13.0-43/include/linux/input.h but it just outputs:

/* THIS FILE IS GENERATED, DO NOT EDIT */

and then exits with exit-code 2. I feel a bit uneasy creating a PR that I can't test, so I'd like to get the script running on my end, if possible. Do you have an idea what's going wrong?

@ndesh26
Copy link
Owner

ndesh26 commented May 28, 2018

To generate the file we need to provide the headers that are part of libevdev. Try with the following command:-

python tools/make-event-names.py evdev-sys/libevdev/include/linux/input-event-codes.h evdev-sys/libevdev/include/linux/input.h

@soenkehahn
Copy link
Contributor Author

Yup, that works, thanks.

@soenkehahn
Copy link
Contributor Author

I've updated the PR. Now it doesn't modify src/enums.rs. However that means that compilation is broken in that state, since src/lib.rs has #[derive(Debug)] on InputEvent and that requires a Debug impl for all these enum types.

I think a good way to deal with generating code that is checked into the repo is this: whenever a PR changes either tools/make-event-names.py or one of the input files, src/enums.rs should also be re-generated and committed as well. So that the generating code and the generated code are always in sync.

Currently the two seem to be already out of sync. Or the script is non-deterministic. I'll probably look into this further, but I wanted to get your input. What do you think?

@ndesh26
Copy link
Owner

ndesh26 commented May 28, 2018

AFAIK the script is deterministic. The out of sync part was my fault as I had updated the libevdev without updating the src/enums.rs. I would like to avoid commits which break the build. You can send the patch with the updated src/enums.rs. I will reupdate it when I bump the libevdev version.

We could have a git hook to check if a change in tools/make-event-names.py is reflected in src/enums.rs. I have not worked with git hooks before so I will look into it once I get some time.

@soenkehahn
Copy link
Contributor Author

I'm not 100% sure, but I think you can't commit git hooks. So it's just something that you can put into your repo on your local machine. But ideally we would run a check like that on CI.

I think it's nice to have a commit history with a separate commit for just bringing src/enums.rs in sync again. I created a PR for that: #5. If you would merge that, I'd update this PR, and then the diff here should look really nice. :)

@soenkehahn
Copy link
Contributor Author

Alright, now I've updated the PR. It compiles and is ready to merge from my side.

@ndesh26 ndesh26 merged commit 4b6aa27 into ndesh26:master May 29, 2018
@soenkehahn soenkehahn deleted the sh/debug-deriving branch May 29, 2018 15:40
@soenkehahn
Copy link
Contributor Author

@ndesh26: Awesome, thanks a lot for merging all the PRs!

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.

None yet

2 participants