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

Disable rmake test inaccessible-temp-dir on riscv64 #126917

Conversation

Hoverbear
Copy link
Contributor

In #126279 the inaccessible-temp-dir test was moved to rmake, I followed up with a 'fix' derived from #126355 in #126707.

That 'fix' was misguided and hiding the true issue of the linker being incorrect for riscv64gc-unknown-linux-gnu (addressed in #126916).

Unfortunately, even with the linker fixed, this test doesn't work. I asked myself why this appeared to work on other platforms and investigated why. Both the containers for armhf-gnu and riscv64gc run their tests as root and have NO_CHANGE_USER set:

This means the tests are run as root. As root, it's perfectly normal and reasonable to violate permission checks this way:

$ sudo mkdir scratch
$ sudo chmod o-w scratch
$ sudo mkdir scratch/backs
$

Because of this, this PR makes the test ignored on riscv64gc for now.

As an alternative, I believe the best long-term strategy would be to not run the tests as root for this job.

Testing

Note

riscv64gc-unknown-linux-gnu is a Tier 2 with Host Tools platform, all tests may not necessarily pass! This change should only ignore inaccessible-temp-dir and not affect other tests.

You can test out the job locally:

mv src/ci/docker/host-x86_64/disabled/riscv64gc-gnu src/ci/docker/host-x86_64/riscv64gc-gnu
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu

@rustbot
Copy link
Collaborator

rustbot commented Jun 24, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 24, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 24, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@Hoverbear Hoverbear changed the title Disable rmake test inaccessible-temp-dir on riscv64 Disable rmake test inaccessible-temp-dir on riscv64 Jun 24, 2024
@workingjubilee
Copy link
Member

@Hoverbear does the test suite fail in CI if we run them as non-root?

@Hoverbear
Copy link
Contributor Author

I'm glad you asked! I'm gonna find out. :)

@Hoverbear
Copy link
Contributor Author

I did some investigation and here are some of the issues that pop up.

Commenting out this is fine:

(You may need to remove the obj/ folder in the repo if you get weird errors.)

After that this line trips up with a permission error, that's relatively trivial to solve by making it rootfs/tmp/testd:

t!(fs::copy(server, rootfs.join("testd")));

Sadly the mkfs call here is the next to trip up, it wants to create a .pwd.lock in a readonly directory:

let mut mkfs = Command::new("mkfs.ext4");
mkfs.arg("-d").arg(rootfs).arg(rootfs_img);
let mut mkfs_child = t!(mkfs.spawn());
assert!(t!(mkfs_child.wait()).success());

[TIMING] core::build_steps::tool::RemoteTestClient { compiler: Compiler { stage: 0, host: x86_64-unknown-linux-gnu }, target: x86_64-unknown-linux-gnu } -- 0.061
1024+0 records in
1024+0 records out
1073741824 bytes (1.1 GB, 1.0 GiB) copied, 1.23445 s, 870 MB/s
mke2fs 1.46.5 (30-Dec-2021)
Discarding device blocks: done                            
Creating filesystem with 262144 4k blocks and 65536 inodes
Filesystem UUID: 9c29c0d6-f579-46a6-93a1-85535823300a
Superblock backups stored on blocks: 
        32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Creating journal (8192 blocks): done
Copying files into the device: do_write_internal: Permission denied while opening ".pwd.lock" to copy
__populate_fs: Permission denied while writing file ".pwd.lock"
mkfs.ext4: Permission denied while populating file system
thread 'main' panicked at src/tools/remote-test-client/src/main.rs:164:5:
assertion failed: t!(mkfs_child.wait()).success()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:11
  local time: Tue Jun 25 20:26:54 UTC 2024
  network time: Tue, 25 Jun 2024 20:26:57 GMT

Using a different workdirectory (since it's called pwd.lock) does not help, setting -e ignore to ignore errors does not allow progress either.

I guess the solution would be to stop using -d, however that opens a new set of problems since we then need to mount the filesystem and copy over the files manually. I guess that's not entirely different than what's done with arm-unknown-linux-gnueabihf though:

thread::spawn(move || add_files(&mut stdin, &rootfs, &rootfs));
t!(io::copy(&mut child.stdout.take().unwrap(), &mut t!(File::create(&rootfs_img))));
assert!(t!(child.wait()).success());
fn add_files(w: &mut dyn Write, root: &Path, cur: &Path) {
for entry in t!(cur.read_dir()) {
let entry = t!(entry);
let path = entry.path();
let to_print = path.strip_prefix(root).unwrap();
t!(write!(w, "{}\u{0}", to_print.to_str().unwrap()));
if t!(entry.file_type()).is_dir() {
add_files(w, root, &path);
}
}
}

@workingjubilee
Copy link
Member

....huh, weird.

@Hoverbear
Copy link
Contributor Author

Hoverbear commented Jun 26, 2024

I realized I could temporarily hack around this by setting this to rw, just to see if there are other issues:

https://github.com/rust-lang/stdarch/blob/df3618d9f35165f4bc548114e511c49c29e1fd9b/ci/run-docker.sh#L35

This results in further issues:

mke2fs 1.46.5 (30-Dec-2021)
Discarding device blocks: done                            
Creating filesystem with 262144 4k blocks and 65536 inodes
Filesystem UUID: e43c16af-4e66-4156-b0c3-f98d8984d124
Superblock backups stored on blocks: 
        32768, 98304, 163840, 229376

Allocating group tables: done                            
Writing inode tables: done                            
Creating journal (8192 blocks): done
Writing superblocks and filesystem accounting information: done

mount: /tmp/scratch: failed to setup loop device for /checkout/obj/build/tmp/rootfs.img.
thread 'main' panicked at src/tools/remote-test-client/src/main.rs:171:5:
assertion failed: t!(mount_child.wait()).success()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Build completed unsuccessfully in 0:00:14
  local time: Wed Jun 26 13:54:42 UTC 2024
  network time: Wed, 26 Jun 2024 13:54:45 GMT

Originating from:

Suffice it to say, I don't think this is a trivial enablement and it may be more suitable for a future PR which impacts both the riscv64-gnu and armhf-gnu targets.

Copy link
Member

@jieyouxu jieyouxu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm fine with ignoring this test for riscv64 for now. Could you convert the Reason into a FIXME? i.e. this is something we should eventually fix in the long term.

@jieyouxu
Copy link
Member

r=me once you add a FIXME in the comment explaining the ignore. Maybe also include some of the steps you tried but did not work in the test comment (helpful for future person trying to enable this test for riscv64).

@bors delegate+

@bors
Copy link
Contributor

bors commented Jun 29, 2024

✌️ @Hoverbear, you can now approve this pull request!

If @jieyouxu told you to "r=me" after making some further change, please make that change, then do @bors r=@jieyouxu

@jieyouxu
Copy link
Member

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 29, 2024
@bors
Copy link
Contributor

bors commented Jul 1, 2024

☔ The latest upstream changes (presumably #127216) made this pull request unmergeable. Please resolve the merge conflicts.

@Hoverbear Hoverbear force-pushed the hoverbear/riscv64-inaccessible-temp-dir-resolution branch 2 times, most recently from 48e99b5 to 1d733f0 Compare July 2, 2024 13:48
@Hoverbear Hoverbear force-pushed the hoverbear/riscv64-inaccessible-temp-dir-resolution branch from 1d733f0 to 8c353cb Compare July 2, 2024 13:50
@Hoverbear
Copy link
Contributor Author

@bors r=@jieyouxu

@bors
Copy link
Contributor

bors commented Jul 2, 2024

📌 Commit 8c353cb has been approved by jieyouxu

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Jul 2, 2024

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 2, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 2, 2024
…ssible-temp-dir-resolution, r=jieyouxu

Disable rmake test `inaccessible-temp-dir` on riscv64

In rust-lang#126279 the `inaccessible-temp-dir` test was moved to rmake, I followed up with a 'fix' derived from rust-lang#126355 in rust-lang#126707.

That 'fix' was misguided and hiding the true issue of the linker being incorrect for `riscv64gc-unknown-linux-gnu` (addressed in rust-lang#126916).

Unfortunately, even with the linker fixed, this test doesn't work. I asked myself why this appeared to work on other platforms and investigated why. Both the containers for `armhf-gnu` and `riscv64gc` run their tests as `root` and have `NO_CHANGE_USER` set:

https://github.com/rust-lang/rust/blob/553a69030e5a086eb3841d020db8c9c463948c72/src/ci/docker/host-x86_64/disabled/riscv64gc-gnu/Dockerfile#L99

This means the tests are run as `root`. As `root`, it's perfectly normal and reasonable to violate permission checks this way:

```bash
$ sudo mkdir scratch
$ sudo chmod o-w scratch
$ sudo mkdir scratch/backs
$
```

Because of this, this PR makes the test ignored on `riscv64gc` for now.

As an alternative, I believe the best long-term strategy would be to not run the tests as `root` for this job.

## Testing

> [!NOTE]
> `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html), all tests may not necessarily pass! This change should only ignore `inaccessible-temp-dir` and not affect other tests.

You can test out the job locally:

```sh
mv src/ci/docker/host-x86_64/disabled/riscv64gc-gnu src/ci/docker/host-x86_64/riscv64gc-gnu
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126403 (Actually report normalization-based type errors correctly for alias-relate obligations in new solver)
 - rust-lang#126803 (Change `asm-comments` to `verbose-asm`, always emit user comments)
 - rust-lang#126917 (Disable rmake test `inaccessible-temp-dir` on riscv64)
 - rust-lang#127050 (Make mtime of reproducible tarballs dependent on git commit)
 - rust-lang#127145 (Add `as_lang_item` to `LanguageItems`, new trait solver)
 - rust-lang#127184 (More refactorings to rustc_interface)
 - rust-lang#127202 (Remove global error count checks from typeck)
 - rust-lang#127233 (Some parser cleanups)
 - rust-lang#127245 (Add a test for `generic_const_exprs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#126403 (Actually report normalization-based type errors correctly for alias-relate obligations in new solver)
 - rust-lang#126803 (Change `asm-comments` to `verbose-asm`, always emit user comments)
 - rust-lang#126917 (Disable rmake test `inaccessible-temp-dir` on riscv64)
 - rust-lang#127050 (Make mtime of reproducible tarballs dependent on git commit)
 - rust-lang#127145 (Add `as_lang_item` to `LanguageItems`, new trait solver)
 - rust-lang#127184 (More refactorings to rustc_interface)
 - rust-lang#127202 (Remove global error count checks from typeck)
 - rust-lang#127233 (Some parser cleanups)
 - rust-lang#127245 (Add a test for `generic_const_exprs`)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#123588 (Stabilize `hint::assert_unchecked`)
 - rust-lang#126403 (Actually report normalization-based type errors correctly for alias-relate obligations in new solver)
 - rust-lang#126917 (Disable rmake test `inaccessible-temp-dir` on riscv64)
 - rust-lang#127115 (unreferenced-used-static: run test everywhere)
 - rust-lang#127204 (Stabilize atomic_bool_fetch_not)
 - rust-lang#127239 (remove unnecessary ignore-endian-big from stack-overflow-trait-infer …)
 - rust-lang#127245 (Add a test for `generic_const_exprs`)
 - rust-lang#127246 (Give remote-test-client a longer timeout)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3e6f6de into rust-lang:master Jul 3, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 3, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 3, 2024
Rollup merge of rust-lang#126917 - ferrocene:hoverbear/riscv64-inaccessible-temp-dir-resolution, r=jieyouxu

Disable rmake test `inaccessible-temp-dir` on riscv64

In rust-lang#126279 the `inaccessible-temp-dir` test was moved to rmake, I followed up with a 'fix' derived from rust-lang#126355 in rust-lang#126707.

That 'fix' was misguided and hiding the true issue of the linker being incorrect for `riscv64gc-unknown-linux-gnu` (addressed in rust-lang#126916).

Unfortunately, even with the linker fixed, this test doesn't work. I asked myself why this appeared to work on other platforms and investigated why. Both the containers for `armhf-gnu` and `riscv64gc` run their tests as `root` and have `NO_CHANGE_USER` set:

https://github.com/rust-lang/rust/blob/553a69030e5a086eb3841d020db8c9c463948c72/src/ci/docker/host-x86_64/disabled/riscv64gc-gnu/Dockerfile#L99

This means the tests are run as `root`. As `root`, it's perfectly normal and reasonable to violate permission checks this way:

```bash
$ sudo mkdir scratch
$ sudo chmod o-w scratch
$ sudo mkdir scratch/backs
$
```

Because of this, this PR makes the test ignored on `riscv64gc` for now.

As an alternative, I believe the best long-term strategy would be to not run the tests as `root` for this job.

## Testing

> [!NOTE]
> `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html), all tests may not necessarily pass! This change should only ignore `inaccessible-temp-dir` and not affect other tests.

You can test out the job locally:

```sh
mv src/ci/docker/host-x86_64/disabled/riscv64gc-gnu src/ci/docker/host-x86_64/riscv64gc-gnu
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 3, 2024
…rustdoc-io-error, r=jieyouxu

Disable rmake test rustdoc-io-error on riscv64gc-gnu

In rust-lang#126917 we disabled `inaccessible-temp-dir` on `riscv64gc-gnu` because the container runs the build as `root` (just like the `armhf-gnu` builds). Tests creating an inaccessible test directory are not possible, since `root` can always touch those directories.

https://github.com/rust-lang/rust/blob/553a69030e5a086eb3841d020db8c9c463948c72/src/ci/docker/host-x86_64/disabled/riscv64gc-gnu/Dockerfile#L99

This means the tests are run as `root`. As `root`, it's perfectly normal and reasonable to violate permission checks this way:

```bash
$ sudo mkdir scratch
$ sudo chmod o-w scratch
$ sudo mkdir scratch/backs
$
```

Because of this, this PR makes the test ignored on `riscv64gc` (just like on `armhf-gnu`) for now.

As an alternative, I believe the best long-term strategy would be to not run the tests as `root` for this job. Some preliminary exploration was done in rust-lang#126917 (comment), however that appears a larger lift.

## Testing

> [!NOTE]
> `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html), all tests may not necessarily pass! This change should only ignore `inaccessible-temp-dir` and not affect other tests.

You can test out the job locally:

```sh
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```

r? `@jieyouxu`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2024
Rollup merge of rust-lang#127280 - ferrocene:hoverbear/disable-rmake-rustdoc-io-error, r=jieyouxu

Disable rmake test rustdoc-io-error on riscv64gc-gnu

In rust-lang#126917 we disabled `inaccessible-temp-dir` on `riscv64gc-gnu` because the container runs the build as `root` (just like the `armhf-gnu` builds). Tests creating an inaccessible test directory are not possible, since `root` can always touch those directories.

https://github.com/rust-lang/rust/blob/553a69030e5a086eb3841d020db8c9c463948c72/src/ci/docker/host-x86_64/disabled/riscv64gc-gnu/Dockerfile#L99

This means the tests are run as `root`. As `root`, it's perfectly normal and reasonable to violate permission checks this way:

```bash
$ sudo mkdir scratch
$ sudo chmod o-w scratch
$ sudo mkdir scratch/backs
$
```

Because of this, this PR makes the test ignored on `riscv64gc` (just like on `armhf-gnu`) for now.

As an alternative, I believe the best long-term strategy would be to not run the tests as `root` for this job. Some preliminary exploration was done in rust-lang#126917 (comment), however that appears a larger lift.

## Testing

> [!NOTE]
> `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html), all tests may not necessarily pass! This change should only ignore `inaccessible-temp-dir` and not affect other tests.

You can test out the job locally:

```sh
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```

r? `@jieyouxu`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 4, 2024
…-testing, r=<try>

Enable `riscv64gc` testing

Together with [email protected], we've been starting to explore improving the state of the `riscv64gc-unknown-linux-gnu` target. Additionally, I'm looking to add support for this platform in [Ferrocene](https://github.com/ferrocene/ferrocene) ([Related PR](ferrocene/ferrocene#618)).

Recently several PRs have landed improving the state of this target:

* rust-lang#125220
* rust-lang#125669
* rust-lang#126355
* rust-lang#126279
* rust-lang#126707
* rust-lang#126916
* rust-lang#126917
* rust-lang#127280

The result has been that `riscv64gc-unknown-linux-gnu` now *should* pass the same CI tests that `x86_64-unknown-linux-gnu` and `aarch64-unknown-linux-gnu` do.

## Testing

> [!NOTE]
> While `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html) (meaning all tests may not necessarily pass) we do need to see all of the tests passing here. Indeed, the point of this PR is to get `riscv64gc-unknown-linux-gnu` into automated testing so the tests can *remain* working.

You can test out the job locally:

```sh
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```

`DEPLOY=1` helps reproduce the CI's environment and also avoids the chance of a `llvm-c/BitReader.h` error (detailed in rust-lang#85424 and rust-lang#56650).

try-job: riscv64gc-gnu
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 20, 2024
…-testing, r=<try>

Enable `riscv64gc-gnu` testing

Together with [email protected], we've been starting to explore improving the state of the `riscv64gc-unknown-linux-gnu` target. Additionally, I'm looking to add support for this platform in [Ferrocene](https://github.com/ferrocene/ferrocene) ([Related PR](ferrocene/ferrocene#618)).

Recently several PRs have landed improving the state of this target:

* rust-lang#125220
* rust-lang#125669
* rust-lang#126355
* rust-lang#126279
* rust-lang#126707
* rust-lang#126916
* rust-lang#126917
* rust-lang#127280
* rust-lang#127967

The result has been that `riscv64gc-unknown-linux-gnu` now *should* pass the same CI tests that `x86_64-unknown-linux-gnu` and `aarch64-unknown-linux-gnu` do.

## Testing

> [!NOTE]
> While `riscv64gc-unknown-linux-gnu` is a [**Tier 2 with Host Tools** platform](https://doc.rust-lang.org/beta/rustc/platform-support.html) (meaning all tests may not necessarily pass) we do need to see all of the tests passing here. Indeed, the point of this PR is to get `riscv64gc-unknown-linux-gnu` into automated testing so the tests can *remain* working.

You can test out the job locally:

```sh
DEPLOY=1 ./src/ci/docker/run.sh riscv64gc-gnu
```

`DEPLOY=1` helps reproduce the CI's environment and also avoids the chance of a `llvm-c/BitReader.h` error (detailed in rust-lang#85424 and rust-lang#56650).

try-job: riscv64gc-gnu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants