-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
FreeBSD: add evdev structures #3756
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @JohnTitor (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
|
Some context: On FreeBSD the evdev headers are not part of the base system, but come from a package named @devnexen You did some FreeBSD PRs already, could you please look at this one too? |
Just quick response here: by definition if it is not part of the system thus has no place in this crate, e.g. libutil is still part of builtin libraries. my 2 cents :) |
Yes, but on Linux these data types are part of libc. Because of this, the consumers of this library that are using these types will fail to compile on FreeBSD. This is the case for the new release of https://github.com/mqudsi/syngesture I think that putting these types here is the most correct solution, but I know nothing of Rust ecosystem. |
I get it, but Linux is Linux. However though, nothing is stopping you define those types in the requested project (or to publish a crate dedicated to those sort of things ?). But hey, I m not the maintainer of libc :) I do not get to decide. |
This would require patching for each project that depends on these types in libc.
This would probably require patching too, although much less? Maybe it is a way to go then? |
Regardless, to make CI work here you would need to add the related dependency. CI at the moment only install what necessaty, it appears, for rustup nothing else. The decision to merge it or not is more @JohnTitor call :) |
I think you need to declare the appropriate evdev-proto header in |
5d44a74
to
6a00f38
Compare
Also note you need to treat struct field such as |
@devnexen thank you for the comments! Maybe you can advise me how to create a flag for checking file in /usr/local/include ? |
Are you able to run tests locally ? |
Yes |
ac3af67
to
f2fb2da
Compare
libc-test/build.rs
Outdated
@@ -1990,6 +1990,9 @@ fn test_freebsd(target: &str) { | |||
cfg.define("_WITH_GETLINE", None); | |||
// Required for making freebsd11_stat available in the headers | |||
cfg.define("_WANT_FREEBSD11_STAT", None); | |||
cfg.define("LIBICONV_PLUG", None); | |||
|
|||
cfg.flag("-I/usr/local/include"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we should use the value of sysctl -n user.localbase
here instead of hardcoding "/usr/local", but I don't know how to do it in Rust.
☔ The latest upstream changes (presumably #3889) made this pull request unmergeable. Please resolve the merge conflicts. |
CC @wulf7 who implemented evdev on FreeBSD and defined evdev-proto policy. |
If this code is FreeBSD-only, you can rely on <dev/evdev/input.h> |
f22a38d
to
38024ae
Compare
pub struct input_event { | ||
pub time: crate::timeval, | ||
pub type_: crate::u_short, | ||
pub code: crate::u_short, | ||
pub value: c_int, | ||
} | ||
|
||
pub struct input_absinfo { | ||
pub value: c_int, | ||
pub minimum: c_int, | ||
pub maximum: c_int, | ||
pub fuzz: c_int, | ||
pub flat: c_int, | ||
pub resolution: c_int, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The integers can use i32
to match https://github.com/freebsd/freebsd-src/blob/38b67578fb4bbf568f7012ca3921a4d15cfe7c5d/sys/dev/evdev/input.h#L51-L65
Thanks for the ping, I didn't realize this was ready (for future reference, @Awoonyaa one nit above and can you add these to Cc @asomers @rustbot author |
The definitions look correct to me. Personally I'd be tempted to expose these under an |
Thanks for taking a look. I have been thinking that modules mirroring C header paths might be worth doing for 1.0, then only reexporting a common subset for backward compatibility. Especially considering this would make our sources easier to navigate and cross reference to the system headers. But indeed we may as well be consistent here. |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
be9882d
to
00e4603
Compare
No description provided.