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

readLink() testcase failed with localized non-english language? #93211

Closed
m1guelperez opened this issue Jan 22, 2022 · 18 comments
Closed

readLink() testcase failed with localized non-english language? #93211

m1guelperez opened this issue Jan 22, 2022 · 18 comments
Assignees
Labels
C-bug Category: This is a bug.

Comments

@m1guelperez
Copy link

m1guelperez commented Jan 22, 2022

Hey!
I am not sure if this is directly a bug on the compiler test case or if it is my machine's fault.
My OS is Windows 11 Pro with version: 10.0.22000 Build 22000.
I built the compiler for the first time, set everything up (toolchains, etc.), and never got an error so far.

./x.py check succeeded instantly as well.
But as soon as I tried to run the following test,
./x.py test --stage 0 library/std
without any changes made to the compiler, 1 test case failed.

The test case fn readLink() in: library\std\src\fs\tests.rs:839:20 is the problem, or to be more precise, the last assert_eq!in the
if cfg!(windows){/.../}

This won't pass:

#[test]
fn read_link() {
    if cfg!(windows) {
        // directory symlink
        assert_eq!(check!(fs::read_link(r"C:\Users\All Users")), Path::new(r"C:\ProgramData"));
        // junction
        assert_eq!(check!(fs::read_link(r"C:\Users\Default User")), Path::new(r"C:\Users\Default"));
        // junction with special permissions
        assert_eq!(check!(fs::read_link(r"C:\Documents and settings\")), Path::new(r"C:\Users"));
    }
    let tmpdir = tmpdir();
    let link = tmpdir.join("link");
    if !got_symlink_permission(&tmpdir) {
        return;
    };
    check!(symlink_file(&"foo", &link));
    assert_eq!(check!(fs::read_link(&link)).to_str().unwrap(), "foo");
}

This will pass:

fn read_link() {
    if cfg!(windows) {
        // directory symlink
        assert_eq!(check!(fs::read_link(r"C:\Users\All Users")), Path::new(r"C:\ProgramData"));
        // junction
        assert_eq!(check!(fs::read_link(r"C:\Users\Default User")), Path::new(r"C:\Users\Default"));
        // junction with special permissions
        assert_eq!(check!(fs::read_link(r"C:\Dokumente und Einstellungen\")), Path::new(r"C:\Users"));
    }
    let tmpdir = tmpdir();
    let link = tmpdir.join("link");
    if !got_symlink_permission(&tmpdir) {
        return;
    };
    check!(symlink_file(&"foo", &link));
    assert_eq!(check!(fs::read_link(&link)).to_str().unwrap(), "foo");
}

I expected to see the test pass, but it seems, that if the junction is not called in English, the test will fail.

Instead, I got the following error (long error below):
thread 'fs::tests::read_link' panicked at 'fs::read_link(r"C:\Documents and settings\") failed with: The system cannot find the file specified. (os error 2)', library\std\src\fs\tests.rs:839:20

Finally, I am not sure if this is an "error" in the test case or if my windows is broken, because I set the origin language (german) to English.

I included my cmd output which shows the junction is in german.
Dokumente und Einstellungen means Documents and Settings in German

Junction

Meta

rustc --version --verbose:

rustc 1.60.0-nightly (777bb86bc 2022-01-20)
binary: rustc
commit-hash: 777bb86bcdbc568be7cff6eeeaaf81a89b4aa50b
commit-date: 2022-01-20
host: x86_64-pc-windows-msvc
release: 1.60.0-nightly
LLVM version: 13.0.0
Backtrace

failures:

---- fs::tests::read_link stdout ----
thread 'fs::tests::read_link' panicked at 'fs::read_link(r"C:\Documents and settings\") failed with: The system cannot find the file specified. (os error 2)', library\std\src\fs\tests.rs:839:20


failures:
    fs::tests::read_link

test result: FAILED. 896 passed; 1 failed; 3 ignored; 0 measured; 0 filtered out; finished in 10.90s

error: test failed, to rerun pass '-p std --lib'


command did not execute successfully: "\\\\?\\C:\\rust\\build\\x86_64-pc-windows-msvc\\stage0\\bin\\cargo.exe" "test" "--target" "x86_64-pc-windows-msvc" "-Zbinary-dep-depinfo" "-j" "20" "--release" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "C:\\rust\\library/test/Cargo.toml" "-p" "std" "--" "--quiet"
expected success, got: exit code: 101

@m1guelperez m1guelperez added the C-bug Category: This is a bug. label Jan 22, 2022
@eddyb
Copy link
Member

eddyb commented Jan 23, 2022

Do you happen to have access to any german-language Windows 10 machines? I'm curious if this localization is new in Windows 11, or pre-existing.

@m1guelperez
Copy link
Author

m1guelperez commented Jan 23, 2022

@eddyb I have a laptop from work which runs Windows 10 Enterprise in German, Version 21H1.

The cmd output is the same, with Dokumente und Einstellungen as junction instead of Documents and settings

@hkratz
Copy link
Contributor

hkratz commented Jan 23, 2022

Weird, this is on my Windows 10 laptop:

C:\Users\xxx>dir /al \
 Volume in drive C is OS
 Volume Serial Number is xxxx-xxxx

 Directory of C:\

17.12.2019  19:15    <JUNCTION>     Documents and Settings [C:\Users]
18.06.2020  06:37    <JUNCTION>     Dokumente und Einstellungen [C:\Users]
18.06.2020  06:37    <JUNCTION>     Programme [C:\Program Files]
               0 File(s)              0 bytes
               3 Dir(s)  1.312.741.294.080 bytes free

It might depend on the system language used for installation. I think we should just disable the assert, if C:\Documents and settings\ does not exist.

@m1guelperez
Copy link
Author

@hkratz Interesting, but yes I think you are right, that it depends on the system language used for installation.
Could I take this issue as my first one?

@eddyb
Copy link
Member

eddyb commented Jan 23, 2022

Wow, looks like this was added almost 6 years ago, in #31630, and not touched since.

I'm surprised nobody ran into this before, seems like it would be an issue with any localized Windows.
Then again, the way I've seen Windows get localized is after-the-fact, in a way that's easy to toggle, and I don't really understand how that could create anything as global as C:\Documents and Settings.

(Another aspect is that I've had to deal with Romanian-localized Windows and I don't ever recall seeing C:\Documente și setări myself, but some googling does find it, so it's not guaranteed?)

Maybe it only happens if the localization was used during installation, or maybe you have a "localized OEM" edition? If this didn't take up time and whatnot, I would consider playing around with Windows VMs just to see what conditions are required to get the file system in that state.


Removing the case from the test seems like it might leave some codepath untested, I wonder what "junction with special permissions" means and if there is some unlocalized one by default on Windows.

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

We can also try creating one if needed. At least CI has admin permissions.

@m1guelperez
Copy link
Author

m1guelperez commented Jan 23, 2022

I attached my Windows version. This should mean it is not an OEM version as far as I know.
winversion

Removing the case from the test seems like it might leave some codepath untested, I wonder what "junction with special
permissions" means and if there is some unlocalized one by default on Windows.

Yes, I would like to know as well, why we need this or maybe we can use another junction for this assert_eq!?

We can also try creating one if needed. At least CI has admin permissions.

Do you mean manually creating a junction called C:\Documents and Settings?
I guess that would be a possible solution but I am not sure if this is a good one since you need admin rights which can be denied, depending on the machine you are working on.

However, I manually created a junction like that:
mklink /J "C:\Documents and settings" "C:\Users"

This would work.

So you guys would not change anything regarding the test case?
Because since the rust community is growing all over the world, I think the problem will remain for localized installations and probably a lot more people will stumble upon that.
So should we maybe add a hint or something like that to the compiler guide?

@ChrisDenton
Copy link
Contributor

I think the best option would be for the test to create a symlink (and/or juncture point) itself, set the permissions on that and then try to read the link. That way the test will run independently of whether a specific directory exists or not.

However, I don't think we currently have any code for setting permissions on Windows so that would need to be written by someone.

@m1guelperez
Copy link
Author

m1guelperez commented Jan 23, 2022

I think the best option would be for the test to create a symlink (and/or juncture point) itself, set the permissions on that and then try to read the link. That way the test will run independently of whether a specific directory exists or not.

I also had that idea in mind. But wouldn't we then have to execute the test itself with admin rights? When the test case set permissions itself?

Because I am not sure if windows allows to run code, which changes permissions, in non admin mode.

But if the requirement for compiler tests is, to have admin rights, then this shouldn't be a problem. But I thought, that compiler tests should run without admin rights, could be a mistake on my site though.

However, I don't think we currently have any code for setting permissions on Windows so that would need to be written by someone.

There she goes, my hope for my first pull request :P

@m1guelperez m1guelperez changed the title readLink() testcase failed with german language? readLink() testcase failed with localized non-english language? Jan 23, 2022
@ChrisDenton
Copy link
Contributor

You can set permissions on ones you created because the user will own them by default. So this shouldn't require admin rights.

There she goes, my hope for my first pull request :P

Hm, perhaps we could temporarily skip that part of the test when the directory isn't found, and add a "FIXME" comment saying someone needs to write a fallback implementation? Though I'd ask @the8472 if that's ok as a temporary measure?

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

The main concern would be that the next CI runner update might silently break this if microsoft decides to not created it on newer windows versions. So it's a tradeoff between running the tests on some non-english machines vs. having this particular case reliably covered in CI.

To have our cake and eat it too we could make the test mandatory in CI but optional on local machines, e.g by checking the CI environment variable.

@m1guelperez
Copy link
Author

To have our cake and eat it too we could make the test mandatory in CI but optional on local machines, e.g by checking the CI environment variable.

Do you mean something like that?

fn snippet() {
    let var = "CI";
    match env::var(var) {
        Ok(val) => if val == "true" {assert_eq!(check!(fs::read_link(r"C:\Documents and settings\")), Path::new(r"C:\Users"));},
        Err(e) => println!("Test case can be skipped"),
    }
}

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

Kind of, but I meant:

  • if the CI var exists then always run the assert
  • if it does not then first check if Documents and settings doesn't exist and only then skip the assert

@m1guelperez
Copy link
Author

fn snippet() {
    let var = "CI";
    match env::var(var) {
        Ok(val) => assert_eq!(check!(fs::read_link(r"C:\Documents and settings\")), Path::new(r"C:\Users")),
        Err(e) => if Path::new(r"C:\Documents and settings\").exists() {
            assert_eq!(check!(fs::read_link(r"C:\Documents and settings\")), Path::new(r"C:\Users"))
        } else { println!("Path does not exist => Skip the test.") },
    }
}

Okay, then something like that this should to the job?

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

Roughly, the details can be discussed in PR review.

@m1guelperez
Copy link
Author

m1guelperez commented Jan 23, 2022

So I can create my first PR? :)

Do I have to claim?

@the8472
Copy link
Member

the8472 commented Jan 23, 2022

Yes, you don't need to ask for permission for that. At worst there'll be some reason it can't be accepted.

@m1guelperez
Copy link
Author

@rustbot claim

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Mar 11, 2022
Fix for localized windows editions in testcase fn read_link() Issue#93211

This PR aims to fix the issue with localized windows versions that do not necessarily have the folder "Documents and settings" in English.

The idea was provided by `@the8472.` We check if the "CI" environment variable is set, then we always check for the "Documents and Settings"-folder, otherwise we check if the folder exists on the local machine, and if not we skip this assert.

Resoles rust-lang#93211.
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.
Projects
None yet
Development

No branches or pull requests

5 participants