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

Ignore run-make tests that use symlink on Windows and adjust symlink wrapper #126846

Closed
wants to merge 2 commits into from

Conversation

jieyouxu
Copy link
Member

@jieyouxu jieyouxu commented Jun 23, 2024

Noticed in #125674 (comment).

Symlink creation is a privileged operation (std::os::windows::fs::symlink_file), it may be enabled on CI, but it's typically not enabled on user machines.

This PR does two things:

  1. We change the support library's symlink wrapper to panic on Windows because user machines typically don't enable priviledged symlink operations. This can manifest in the test passing in CI but failing on a local machine, as is observed in the linked PR comment.
  2. We update the tests {symlinked-extern, symlinked-rlib, symlinked-libraries} to //@ ignore-windows since they rely on symlink functionality (they were //@ ignore-windows originally).

cc @petrochenkov since you ran into the failing tests.

jieyouxu added 2 commits June 23, 2024 00:21
symlink creation is a priviledged operation on Windows that is normally
not enabled on user machines. It is enabled in CI, however. So let's
not make it easier to create symlinks on Windows.
@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2024

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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 23, 2024
@jieyouxu jieyouxu changed the title Ignore run-make tests that use symlink on Windows Ignore run-make tests that use symlink on Windows and adjust symlink wrapper Jun 23, 2024
@ChrisDenton
Copy link
Member

We should ideally still be running these tests in CI. I'm somewhat uncomfortable with outright ignoring them on Windows. In the standard library we use the CI environment variable to know if a test can be allowed to fail. I'm not sure if a similar thing could be done in run-make tests.

Also many developers do enable developer mode on their computer, so it'd be good if the tests did run locally in that situation. But I think that's a less pressing issue.

@jieyouxu
Copy link
Member Author

jieyouxu commented Jun 23, 2024

I'm not sure if a similar thing could be done in run-make tests.

I suppose we could add a directive //@ only-ci, which is... lol.

Also many developers do enable developer mode on their computer, so it'd be good if the tests did run locally in that situation.

I'm not 100% sure on how to handle this, as in try to do a (maybe cached) symlink creation permission test (maybe in compiletest?)? Or something like (this isn't pretty, just illustrative) in the support library

static HAS_WINDOWS_SYMLINK_PERMS: OnceLock<bool> = OnceLock::new();

fn symlink_wrapper(...) -> ... {
	let has_symlink_perms = HAS_WINDOWS_SYMLINK_PERMS.get_or_init(|| {
        let res = /* try do symlink */;
        let can_symlink = res.is_ok();
        /* remove the symlink */
        can_symlink
	});
    /* do the symlink */
}

(It's a heuristic and not robust to TOCTOU problems, but yes)

@workingjubilee
Copy link
Member

clearly we should add

//@ needs-symlinks

@petrochenkov
Copy link
Contributor

@jieyouxu
got_symlink_permission in std tests doesn't even try to cache, just tries to make some symlink every time.

@ChrisDenton
Copy link
Member

clearly we should add

//@ needs-symlinks

That seems like it would be easy to do? Easy to cache even, The downside is that there's no enforcement. E.g. in CI there's no difference between using needs-symlinks or not.

@petrochenkov
Copy link
Contributor

E.g. in CI there's no difference between using needs-symlinks or not.

Not necessarily, e.g. needs-llvm-components allows to ignore tests locally, but still requires all the components to be present on CI.

@Mark-Simulacrum
Copy link
Member

r? @ChrisDenton (but seems right to me in direction + needs-symlink as the path)

@jieyouxu
Copy link
Member Author

Closing this PR for #126862 which implements the cleaner needs-symlinks approach.

@jieyouxu jieyouxu closed this Jun 23, 2024
@jieyouxu jieyouxu deleted the fix-symlink-rmake branch June 23, 2024 18:18
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jun 23, 2024
Add needs-symlink directive to compiletest

This is an alternative to rust-lang#126846 that allows running symlink tests on Windows in CI but will ignore them locally if symlinks aren't available. A future improvement would be to check that the `needs-symlink` directive is used in rmake files that call `create_symlink` but this is just a quick PR to unblock Windows users who want to run tests locally without enabling symlinks.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 23, 2024
Rollup merge of rust-lang#126862 - ChrisDenton:needs-symlink, r=jieyouxu

Add needs-symlink directive to compiletest

This is an alternative to rust-lang#126846 that allows running symlink tests on Windows in CI but will ignore them locally if symlinks aren't available. A future improvement would be to check that the `needs-symlink` directive is used in rmake files that call `create_symlink` but this is just a quick PR to unblock Windows users who want to run tests locally without enabling symlinks.
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-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants