-
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
Add and fix tests for {i686, aarch64}-linux-android targets #538
Conversation
- some tests are failing - remove readlink, timegm and sig* functions in favor of the unix/mod.rs definitions - remove time64_t (it is not defined for aarch64) - move some definitions to android/b32.rs and create appropriated definitions in android/b64.rs
(rust_highfive has picked a reviewer for you, use r? to override) |
src/unix/mod.rs
Outdated
// Android has some weirdness in this definition | ||
// See weirdness in src/unix/notbsd/android/mod.rs | ||
#[cfg(any(not(target_os = "android"), // " if " -- appease style checker | ||
not(target_arch = "aarch64")))] |
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.
I unfortunately regret the block below this, but could we just duplicate these functions in lower modules to avoid this #[cfg]
attribute?
src/unix/notbsd/android/mod.rs
Outdated
pub type c_char = u8; | ||
pub type wchar_t = u32; | ||
} | ||
} |
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.
Could this be pushed down further?
src/unix/notbsd/android/mod.rs
Outdated
pub const O_DIRECTORY: ::c_int = 0x4000; | ||
pub const O_NOFOLLOW: ::c_int = 0x8000; | ||
} | ||
} |
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.
Could this also be pushed down further?
Looks great to me, thanks so much for taking the charge on this! Overall it'd be great to reduce the |
- Copy 17 functions definitions from src/unix/mod.rs to src/unix/bsd/mod.rs src/unix/haiku/mod.rs src/unix/notbsd/linux/mod.rs and src/unix/solaris/mod.rs - Add some functions to android that was cfged out - Remove cf* and tc* functions implementations for android (they are available with api >= 12, which was release in 2011)
☔ The latest upstream changes (presumably #541) made this pull request unmergeable. Please resolve the merge conflicts. |
@alexcrichton All cfg_if! removed. We got some network issues in the tests. |
@malbarbo maybe try |
@alexcrichton All green now. @kamalmarhubi Thanks for the tip. It worked. |
@@ -727,74 +712,6 @@ pub const NLA_TYPE_MASK: ::c_int = !(NLA_F_NESTED | NLA_F_NET_BYTEORDER); | |||
pub const SIGEV_THREAD_ID: ::c_int = 4; | |||
|
|||
f! { | |||
pub fn sigemptyset(set: *mut sigset_t) -> ::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.
It looks like these functions may not have survived a rebase by accident?
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.
These are removed purposely. The old tests used android-9 api, which do not have this functions. The next api version distributed with android ndk is android-12 (release in 2011 https://en.wikipedia.org/wiki/Android_version_history), which define these functions. The definitions used by android are the same of others unix (src/unix/mod.rs).
I can see that this can be a breaking change... Should I return these implementations? (this will require moving definitions out of src/unix/mod.rs)
Keeping our own implementations of these functions seems odd.
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.
Ah ok, in that case sounds great to me!
Android-9 is such an ancient NDK that it seems fine to not require support for that here. If project want that level of compatibility (e.g. libstd) we can vendor the impls ourselves.
@bors: r+ |
📌 Commit b2791db has been approved by |
Add and fix tests for {i686, aarch64}-linux-android targets I think that these changes do not breaks compatibility. There are some types and constants changes to i686 and aarch64, but I see these changes as bug fixes instead of breaking changes. Also the type time64_t was remove from aarch64 because it is not defined in this arch. Fixes #536
💔 Test failed - status-travis |
Failures look related to yesterday's S3 outage:
|
@malbarbo if you get to it before @alexcrichton retries, you could rebase and force push. Push will get Travis to run, rebase will allow homu to use the cached Travis success so long as no other PR is merged in the meantime. Hopefully we can get this out today! |
I can't even see the failures, but I'll take your word for it! @bors: retry |
Add and fix tests for {i686, aarch64}-linux-android targets I think that these changes do not breaks compatibility. There are some types and constants changes to i686 and aarch64, but I see these changes as bug fixes instead of breaking changes. Also the type time64_t was remove from aarch64 because it is not defined in this arch. Fixes #536
☀️ Test successful - status-appveyor, status-travis |
Even if it is unlikely to happen the VSX instructions can be executed in 32bit mode just as well.
I think that these changes do not breaks compatibility.
There are some types and constants changes to i686 and aarch64, but I see these changes as bug fixes instead of breaking changes. Also the type time64_t was remove from aarch64 because it is not defined in this arch.
Fixes #536