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

[QNX7.1] std::fs::tests::symlink_hard_link broken on main #129895

Closed
japaric opened this issue Sep 2, 2024 · 4 comments · Fixed by #130248
Closed

[QNX7.1] std::fs::tests::symlink_hard_link broken on main #129895

japaric opened this issue Sep 2, 2024 · 4 comments · Fixed by #130248
Labels
C-bug Category: This is a bug. O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system P-low Low priority regression-untriaged Untriaged performance or correctness regression. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@japaric
Copy link
Member

japaric commented Sep 2, 2024

Ferrocene CI has detected that this test was broken by #127897 . Specifically, by the change in library/std/src/sys/pal/unix/fs.rs, shown below:

diff --git a/library/std/src/sys/pal/unix/fs.rs b/library/std/src/sys/pal/unix/fs.rs
index 7fa147c9754..fc9d7e98883 100644
--- a/library/std/src/sys/pal/unix/fs.rs
+++ b/library/std/src/sys/pal/unix/fs.rs
@@ -1717,7 +1717,7 @@ pub fn link(original: &Path, link: &Path) -> io::Result<()> {
     run_path_with_cstr(original, &|original| {
         run_path_with_cstr(link, &|link| {
             cfg_if::cfg_if! {
-                if #[cfg(any(target_os = "vxworks", target_os = "redox", target_os = "android", target_os = "espidf", target_os = "horizon", target_os = "vita", target_os = "nto"))] {
+                if #[cfg(any(target_os = "vxworks", target_os = "redox", target_os = "android", target_os = "espidf", target_os = "horizon", target_os = "vita"))] {
                     // VxWorks, Redox and ESP-IDF lack `linkat`, so use `link` instead. POSIX leaves
                     // it implementation-defined whether `link` follows symlinks, so rely on the
                     // `symlink_hard_link` test in library/std/src/fs/tests.rs to check the behavior.

Test output:

---- fs::tests::symlink_hard_link stdout ----
thread 'fs::tests::symlink_hard_link' panicked at std/src/fs/tests.rs:1473:5:
assertion failed: check!(fs::symlink_metadata(tmpdir.join("hard_link"))).file_type().is_symlink()

Reverting that single line diff fixes the test for the QNX7.1 targets, e.g. arch64-unknown-nto-qnx710 and x86_64-pc-nto-qnx710

@nyurik can the change be reverted or does QNX7.0 need to use link, instead of linkat, here? from looking at libc, it appears that both link and linkat are available on QNX7.0 so reverting the change should at least not cause compilation or linking errors. if the case is the latter, then we should use cfg(target_env = "nto70") in addition to cfg(target_os = "nto").

@japaric japaric added the C-bug Category: This is a bug. label Sep 2, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Sep 2, 2024
@lolbinarycat lolbinarycat added the regression-untriaged Untriaged performance or correctness regression. label Sep 2, 2024
@rustbot rustbot added the I-prioritize Issue: Indicates that prioritization has been requested for this issue. label Sep 2, 2024
@saethlin saethlin added O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system T-libs Relevant to the library team, which will review and decide on the PR/issue. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Sep 2, 2024
@nyurik
Copy link
Contributor

nyurik commented Sep 2, 2024

@japaric thanks for the report! I will need to re-test it for QNX 7.0, to see if it is a 7.0 requirement (i don't recall why this was required initially, but clearly I ran into some issues with it). I also plan to do a similar automation to ensure 7.0 builds run without problems on our hardware. Is there a list of tests you see failing on 7.1 that are OK to ignore? I see a list here - is this the most relevant one? Thx!

@japaric
Copy link
Member Author

japaric commented Sep 3, 2024

we do not ignore any single unit test at the moment and we run library (e.g. libstd) tests as well as (cross) compilation tests using TEST_DEVICE_ADDR. we don't pass --exclude to x.py but instead pass the list of test suites as arguments.

we currently run these test suites:

$ # run these from the root of the ferrocene/ferrocene repo

$ ferrocene/ci/split-tasks.py test:library
library/alloc library/core library/test

$ ferrocene/ci/split-tasks.py test:library-std
library/std

$ ferrocene/ci/split-tasks.py qnx:compiletest-no-only-hosts
tests/assembly tests/codegen tests/codegen-units tests/coverage tests/crashes tests/debuginfo tests/incremental tests/mir-opt tests/run-make tests/run-pass-valgrind tests/ui

@apiraino
Copy link
Contributor

apiraino commented Sep 3, 2024

WG-prioritization assigning priority (Zulip discussion).

@rustbot label -I-prioritize +P-low

@rustbot rustbot added P-low Low priority and removed I-prioritize Issue: Indicates that prioritization has been requested for this issue. labels Sep 3, 2024
@nyurik
Copy link
Contributor

nyurik commented Sep 11, 2024

fixed in #130248

@bors bors closed this as completed in b4201d3 Sep 12, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 12, 2024
Rollup merge of rust-lang#130248 - nyurik:fix-129895, r=workingjubilee

Limit `libc::link` usage to `nto70` target only, not NTO OS

It seems QNX 7.0 does not support `linkat` at all (most tests were failing). Limiting to QNX 7.0 only, while using `linkat` for the future versions seems like the right path forward (tested on 7.0).

Fixes rust-lang#129895

CC: `@japaric` `@flba-eb` `@saethlin`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. O-neutrino OS: QNX Neutrino, a POSIX-compatible real-time operating system P-low Low priority regression-untriaged Untriaged performance or correctness regression. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants