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

ANDROID: Add syncfs API in liblibc #3060

Merged
merged 1 commit into from
Jan 31, 2023

Conversation

shikhapanwar
Copy link

@shikhapanwar shikhapanwar commented Jan 9, 2023

This is required to sync everything in a single filesystem. Other solutions like sync() flushes all filesystems which is unnecessary, it is also impractical to call fsync on all files of the filesystem.

@rustbot
Copy link
Collaborator

rustbot commented Jan 9, 2023

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) soon.

Please see the contribution instructions for more information.

@JohnTitor
Copy link
Member

Thanks! @bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2023

📌 Commit c049385 has been approved by JohnTitor

It is now in the queue for this repository.

@qwandor
Copy link
Contributor

qwandor commented Jan 17, 2023

Hi, what are the next steps to get this merged and released? It looks like it's been in the queue for a while now with no change.

@JohnTitor
Copy link
Member

Seems bors didn't anything to merge, @bors retry

@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 17, 2023

📌 Commit c049385 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 17, 2023

⌛ Testing commit c049385 with merge 1ac1829...

bors added a commit that referenced this pull request Jan 17, 2023
ANDROID: Add syncfs API in liblibc

This is required to sync everything in a single filesystem. Other solutions like sync() flushes all filesystems which is unnecessary, it is also impractical to call fsync on all files of the filesystem.
@bors
Copy link
Contributor

bors commented Jan 17, 2023

💔 Test failed - checks-actions

@shikhapanwar
Copy link
Author

The test Docker Linux Tier2 (arm-linux-androideabi) fails with error:
CANNOT LINK EXECUTABLE "/data/local/tmp/main-d701c417e8b08db6": cannot locate symbol "syncfs" referenced by "/data/local/tmp/main-d701c417e8b08db6"...
Any ideas where else I need to add the symbol or what tests to modify?

@JohnTitor
Copy link
Member

syncfs is added in API 28 but our ARM targets on CI use API 24:

arm | armv7)
api=24
image="system-images;android-${api};default;armeabi-v7a"
;;
aarch64)
api=24
image="system-images;android-${api};google_apis;arm64-v8a"

Let's ignore on ARM by

  1. adding an arm var like let arm = target.contains("arm") || target.contains("aarch64");:
    let x86 = target.contains("i686") || target.contains("x86_64");
  2. adding a new arm like the below here:

    libc/libc-test/build.rs

    Lines 1779 to 1815 in 216a428

    cfg.skip_fn(move |name| {
    // skip those that are manually verified
    match name {
    // FIXME: https://github.com/rust-lang/libc/issues/1272
    "execv" | "execve" | "execvp" | "execvpe" | "fexecve" => true,
    // There are two versions of the sterror_r function, see
    //
    // https://linux.die.net/man/3/strerror_r
    //
    // An XSI-compliant version provided if:
    //
    // (_POSIX_C_SOURCE >= 200112L || _XOPEN_SOURCE >= 600) && ! _GNU_SOURCE
    //
    // and a GNU specific version provided if _GNU_SOURCE is defined.
    //
    // libc provides bindings for the XSI-compliant version, which is
    // preferred for portable applications.
    //
    // We skip the test here since here _GNU_SOURCE is defined, and
    // test the XSI version below.
    "strerror_r" => true,
    "reallocarray" => true,
    "__system_property_wait" => true,
    // Added in API level 30, but tests use level 28.
    "mlock2" => true,
    // Added in glibc 2.25.
    "getentropy" => true,
    // Added in API level 28, but some tests use level 24.
    "getrandom" => true,
    _ => false,
    }
    });
// Added in API level 28:
"syncfs" if arm => true

@shikhapanwar
Copy link
Author

Thanks @JohnTitor ! I made the changes and all checks now pass (:
Now, presumably, another @bors r+ would be required.

@bors
Copy link
Contributor

bors commented Jan 19, 2023

@shikhapanwar: 🔑 Insufficient privileges: Not in reviewers

@JohnTitor
Copy link
Member

Thanks! Let's check again, @bors r+

@bors
Copy link
Contributor

bors commented Jan 20, 2023

📌 Commit fde1ddf has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 20, 2023

⌛ Testing commit fde1ddf with merge ad91645...

bors added a commit that referenced this pull request Jan 20, 2023
ANDROID: Add syncfs API in liblibc

This is required to sync everything in a single filesystem. Other solutions like sync() flushes all filesystems which is unnecessary, it is also impractical to call fsync on all files of the filesystem.
@bors
Copy link
Contributor

bors commented Jan 20, 2023

💔 Test failed - checks-actions

@JohnTitor
Copy link
Member

 CANNOT LINK EXECUTABLE "/checkout/target/x86_64-linux-android/debug/deps/main-d677790ea659c418": cannot locate symbol "syncfs" referenced by "/checkout/target/x86_64-linux-android/debug/deps/main-d677790ea659c418"...
libc: CANNOT LINK EXECUTABLE "/checkout/target/x86_64-linux-android/debug/deps/main-d677790ea659c418": cannot locate symbol "syncfs" referenced by "/checkout/target/x86_64-linux-android/debug/deps/main-d677790ea659c418"...

Uhm, it's unexpected but something on x86_64 emulator is old, I guess. Let's ignore the test unconditionally.

@shikhapanwar
Copy link
Author

Done. Thanks. Will require attempt at @bors r+ I presume

@bors
Copy link
Contributor

bors commented Jan 30, 2023

@shikhapanwar: 🔑 Insufficient privileges: Not in reviewers

This is required to sync everything in a single filesystem. Other
solutions like sync() flushes all filesystems which is unnecessary, it
is also impractical to call fsync on all files of the filesystem.

This patch also excludes syncfs for arm from CI.
@JohnTitor
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Jan 30, 2023

📌 Commit ca8f072 has been approved by JohnTitor

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jan 30, 2023

⌛ Testing commit ca8f072 with merge af6e852...

bors added a commit that referenced this pull request Jan 30, 2023
ANDROID: Add syncfs API in liblibc

This is required to sync everything in a single filesystem. Other solutions like sync() flushes all filesystems which is unnecessary, it is also impractical to call fsync on all files of the filesystem.
@bors
Copy link
Contributor

bors commented Jan 30, 2023

💔 Test failed - checks-cirrus-freebsd-14

@shikhapanwar
Copy link
Author

I cannot possibly explain where ld-elf.so.1: /usr/local/lib/libcurl.so.4: Undefined symbol "nghttp2_option_set_no_rfc9113_leading_and_trailing_ws_validation" is coming from for nightly x86_64-unknown-freebsd-14. Have you seen it before?

@JohnTitor
Copy link
Member

That's unrelated to this PR and I've fixed the issue on master, let's retry
@bors retry

@bors
Copy link
Contributor

bors commented Jan 31, 2023

⌛ Testing commit ca8f072 with merge 106b57a...

@bors
Copy link
Contributor

bors commented Jan 31, 2023

☀️ Test successful - checks-actions, checks-cirrus-freebsd-12, checks-cirrus-freebsd-13, checks-cirrus-freebsd-14
Approved by: JohnTitor
Pushing 106b57a to master...

@bors bors merged commit 106b57a into rust-lang:master Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants