Fix for localized windows editions in testcase fn read_link() Issue#93211 #93283

m1guelperez · 2022-01-24T23:58:34Z

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 #93211.

rust-highfive · 2022-01-24T23:58:36Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @kennytm (or someone else) soon.

Please see the contribution instructions for more information.

hkratz

Thanks for working on this!

You need to make sure that your PR does not contain any merge commits. Rebasing your branch on master should automatically remove the merge commit. See the Git documentation for details.

P.S. This is not an "official" review, I am just a contributor, not a team member.

library/std/src/fs/tests.rs

m1guelperez · 2022-01-25T10:25:59Z

@hkratz Thanks for the first tips.
Do I have to create a separate PR when I rebased or is it possible to update this one?

the8472 · 2022-01-25T10:34:57Z

A force-push will update the PR. See also the relevant dev guide sections:
https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy
https://rustc-dev-guide.rust-lang.org/git.html#rebasing

m1guelperez · 2022-01-25T11:34:31Z

Sorry I am stuck.

I can not pull changes from the rust master (not my fork).
While executing: git pull upstream master --ff-only I get the following error:

From https://github.com/rust-lang/rust
 * branch                master     -> FETCH_HEAD
fatal: Not possible to fast-forward, aborting.

What I did before:
git remote add upstream https://github.com/rust-lang/rust.git
On my fork, I created an issue branch, with:
git checkout -b issue-93211-fix

where I updated the fix and pushed it back to my master with:

git add .\library\std\src\fs\tests.rs
git commit -m "Resolved typos and deleted unused variable"
git push
git checkout master
git rebase issue-93211-fix

Which ended with:
Successfully rebased and updated refs/heads/master.

In MY git repo from the fork I now find the following:

This branch is 4 commits ahead, 40 commits behind rust-lang:master.
So before I commit to my master branch now, I should rebase as far as I understood.

However if I now execute:
git rebase master I will get: Current branch master is up to date.
If I try to pull the changes from the rust-lang:master with:
git pull upstream master --ff-only

I get:

remote: Counting objects: 100% (10/10), done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 15 (delta 7), reused 8 (delta 7), pack-reused 5
Unpacking objects: 100% (15/15), 18.45 KiB | 331.00 KiB/s, done.
From https://github.com/rust-lang/rust
 * branch                    master     -> FETCH_HEAD
   df368ae457c..17dfae79bbc  master     -> upstream/master
fatal: Not possible to fast-forward, aborting.

I am not sure how to resolve this, without making my branch just more of a mess.
If it would be easier for you, you can also hit me up on Zulip, username: Anon.
I don't want to spam too many commits to the branch. If this is already too much, I can close the PR and create a new one.

the8472 · 2022-01-25T12:15:50Z

My rebase workflow looks like this:

git switch issue-93211-fix # change to branch, if not already on it
git fetch upstream # get the most recent commits from the rust-lang repo rather than your own github fork, without updating any local branches
git rebase upstream/master # rebase on the remote master branch, not your local master, you can update the local master later
# fix conflicts here if necessary
git push --force # overwrite remote branch on your github fork with your local branch

git pull is only needed to get your local master branch in sync with the remote one, it also does a fetch.

To visualize current git state I have this in my .gitconfig, which can then be invoked as git graph:

[alias]
graph = log --graph --all --oneline --date=format:'%Y-%m-%d %H:%M' --format=format:'%C(auto)%h %C(green)%cd%C(auto) %s%d'

m1guelperez · 2022-01-25T12:36:57Z

My rebase workflow looks like this:

git switch issue-93211-fix # change to branch, if already on it
git fetch upstream # get the most recent commits from the rust-lang repo rather than your own github fork, without updating any local branches
git rebase upstream/master # rebase on the remote master branch, not your local master, you can update the local master later
# fix conflicts here if necessary
git push --force # overwrite remote branch on your github fork with your local branch

Okay, I followed your steps till here and everything went fine so far.

Now I have to fix This branch is 4 commits ahead, 40 commits behind rust-lang:master. on my master branch somehow correct?

Thanks to Zulip everything should be alright now.

hkratz

Those lines are also not part of your change.

library/std/src/fs/tests.rs

m1guelperez · 2022-01-25T15:46:14Z

@hkratz I think it should be correct now, hopefully, it went well this time.

Dylan-DPC · 2022-03-01T12:12:50Z

r? @Mark-Simulacrum

Mark-Simulacrum · 2022-03-02T19:39:48Z

It seems unfortunate to have a CI gate on this test; I wonder if there's a better path that is not localized (for example) that we could use.

@rustbot ping windows -- is there a better path for us to use here? Is there something we could check (better than a CI variable) to confirm that we're on an English-localized system perhaps?

Mark-Simulacrum · 2022-03-02T21:21:15Z

@rustbot ping windows

rustbot · 2022-03-02T21:21:17Z

Hey Windows Group! This bug has been identified as a good "Windows candidate".
In case it's useful, here are some instructions for tackling these sorts of
bugs. Maybe take a look?
Thanks! <3

cc @arlosi @danielframpton @gdr-at-ms @kennykerr @luqmana @lzybkr @nico-abram @retep998 @rylev @sivadeilra @wesleywiser

m1guelperez · 2022-03-03T11:14:09Z

It seems unfortunate to have a CI gate on this test; I wonder if there's a better path that is not localized (for example) that we could use.

Thanks for the feedback so far!
I feel like there are several workarounds, but none is "really nice".

First, there is the one with the CI gate, which I actually liked, because I thought it would be the best compromise.

Another solution would be to create the folder and instantly delete it after the test is over.
However, this needs admin rights iirc.

An absolute overkill solution would be to make a map, which contains the "Documents and Settings" folder in every language, and then simply make a lookup.

I also googled a bit to see if we could easily check in which language the initial windows installation was, but I did not find anything valuable.

But yes, your idea regarding a non-localized path would be perfect!

Mark-Simulacrum · 2022-03-07T22:13:57Z

library/std/src/fs/tests.rs

-        assert_eq!(check!(fs::read_link(r"C:\Documents and Settings\")), Path::new(r"C:\Users"));
+        // Since not all localized windows versions contain the folder "Documents and settings" in english,
+        // we will briefly check, if it exists and otherwise skip the test. Except during CI we will always execute the test.
+        if Path::new(r"C:\Documents and settings\").exists() || env::var_os("CI").is_some() {


It looks like this might've lowercased Settings by accident? It should probably be "Settings" still. (Also in the assert).

Yes, this is correct. It was an accident and should be in uppercase as well.

Mark-Simulacrum · 2022-03-07T22:15:01Z

I'm inclined to move ahead with this PR (almost as-is, modulo the nit), and we can reconsider if it becomes a pain point in the future -- perhaps to just dropping the test altogether.

m1guelperez · 2022-03-07T22:37:15Z

I'm inclined to move ahead with this PR (almost as-is, modulo the nit), and we can reconsider if it becomes a pain point in the future -- perhaps to just dropping the test altogether.

Okay, so for now, you think it is not a big deal and therefore we shouldn't change this test currently?
Should I close this PR then or let it open?

Mark-Simulacrum · 2022-03-07T22:50:44Z

If you fix the nit, then I will go ahead and approve the PR; if for some reason it causes us problems we can reconsider at that point the best course of action.

m1guelperez · 2022-03-07T23:25:53Z

Okay just to clarify, because I am confused of the word "nit".

I guess you mean when I fix the typo regarding the "Settings" correct?

For sure, I try to fix that asap but I have to check if my laptop is able to handle, compiling the whole compiler, since I can't access my main pc for 1 month.

I will try it out tomorrow!

Mark-Simulacrum · 2022-03-07T23:37:29Z

Feel free to just amend the commit; no need to test locally. If you could squash afterwards into one commit as well that would be good.

m1guelperez · 2022-03-08T09:18:30Z

Feel free to just amend the commit; no need to test locally. If you could squash afterwards into one commit as well that would be good.

Hey!
I fixed the typos and tried to squash it in one commit. Please let me know, if everything worked out, because I am still not that experienced with git.

wesleywiser · 2022-03-08T15:23:55Z

is there a better path for us to use here? Is there something we could check (better than a CI variable) to confirm that we're on an English-localized system perhaps?

I'm not aware of a better approach, unfortunately.

Mark-Simulacrum · 2022-03-11T12:14:00Z

@bors r+ rollup

Thanks for your patience @m1guelperez, this looks great. I've approved it now and it will go into the merge queue before actually merging.

bors · 2022-03-11T12:14:02Z

📌 Commit b795ae5 has been approved by Mark-Simulacrum

m1guelperez · 2022-03-11T12:35:15Z

Thanks for your patience @m1guelperez, this looks great. I've approved it now and it will go into the merge queue before actually merging.

No problem! Glad to help and can't wait to make my next contribution!

Thanks to the rust community for being always helpful especially as a beginner!

See you. :-)

Rollup of 5 pull requests Successful merges: - rust-lang#93283 (Fix for localized windows editions in testcase fn read_link() Issue#93211) - rust-lang#94592 (Fallback to top-level config.toml if not present in current directory, and remove fallback for env vars and CLI flags) - rust-lang#94776 (Optimize ascii::escape_default) - rust-lang#94840 (update `replace_bound_vars_with_placeholders` doc comment) - rust-lang#94842 (Remove unnecessary try_opt for operations that cannot fail) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned kennytm Jan 24, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 24, 2022

hkratz reviewed Jan 25, 2022

View reviewed changes

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

m1guelperez force-pushed the master branch from d0d72f2 to 2be645a Compare January 25, 2022 14:01

hkratz reviewed Jan 25, 2022

View reviewed changes

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

library/std/src/fs/tests.rs Outdated Show resolved Hide resolved

m1guelperez force-pushed the master branch from 2be645a to 2cb0bdd Compare January 25, 2022 15:45

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2022

rust-highfive assigned Mark-Simulacrum and unassigned kennytm Mar 1, 2022

This comment was marked as resolved.

Sign in to view

rustbot added the O-windows Operating system: Windows label Mar 2, 2022

Mark-Simulacrum reviewed Mar 7, 2022

View reviewed changes

Fix for issue rust-lang#93283

b795ae5

m1guelperez force-pushed the master branch from 87883db to b795ae5 Compare March 8, 2022 09:16

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-review Status: Awaiting review from the assignee but also interested parties. labels Mar 11, 2022

Dylan-DPC mentioned this pull request Mar 11, 2022

Rollup of 5 pull requests #94845

Merged

bors merged commit 7189fce into rust-lang:master Mar 11, 2022

rustbot added this to the 1.61.0 milestone Mar 11, 2022

the8472 mentioned this pull request Mar 20, 2022

Skip a test if symlink creation is not possible #95114

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for localized windows editions in testcase fn read_link() Issue#93211 #93283

Fix for localized windows editions in testcase fn read_link() Issue#93211 #93283

m1guelperez commented Jan 24, 2022 •

edited by Mark-Simulacrum

Loading

rust-highfive commented Jan 24, 2022

hkratz left a comment •

edited

Loading

m1guelperez commented Jan 25, 2022

the8472 commented Jan 25, 2022

This comment has been minimized.

m1guelperez commented Jan 25, 2022 •

edited

Loading

the8472 commented Jan 25, 2022 •

edited

Loading

m1guelperez commented Jan 25, 2022 •

edited

Loading

hkratz left a comment

m1guelperez commented Jan 25, 2022

Dylan-DPC commented Mar 1, 2022

Mark-Simulacrum commented Mar 2, 2022

This comment was marked as resolved.

Mark-Simulacrum commented Mar 2, 2022

rustbot commented Mar 2, 2022

m1guelperez commented Mar 3, 2022 •

edited

Loading

Mark-Simulacrum Mar 7, 2022

m1guelperez Mar 7, 2022

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 7, 2022

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 7, 2022 •

edited

Loading

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 8, 2022 •

edited

Loading

wesleywiser commented Mar 8, 2022

Mark-Simulacrum commented Mar 11, 2022

bors commented Mar 11, 2022

m1guelperez commented Mar 11, 2022 •

edited

Loading

Fix for localized windows editions in testcase fn read_link() Issue#93211 #93283

Fix for localized windows editions in testcase fn read_link() Issue#93211 #93283

Conversation

m1guelperez commented Jan 24, 2022 • edited by Mark-Simulacrum Loading

rust-highfive commented Jan 24, 2022

hkratz left a comment • edited Loading

Choose a reason for hiding this comment

m1guelperez commented Jan 25, 2022

the8472 commented Jan 25, 2022

This comment has been minimized.

m1guelperez commented Jan 25, 2022 • edited Loading

the8472 commented Jan 25, 2022 • edited Loading

m1guelperez commented Jan 25, 2022 • edited Loading

hkratz left a comment

Choose a reason for hiding this comment

m1guelperez commented Jan 25, 2022

Dylan-DPC commented Mar 1, 2022

Mark-Simulacrum commented Mar 2, 2022

This comment was marked as resolved.

Mark-Simulacrum commented Mar 2, 2022

rustbot commented Mar 2, 2022

m1guelperez commented Mar 3, 2022 • edited Loading

Mark-Simulacrum Mar 7, 2022

Choose a reason for hiding this comment

m1guelperez Mar 7, 2022

Choose a reason for hiding this comment

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 7, 2022

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 7, 2022 • edited Loading

Mark-Simulacrum commented Mar 7, 2022

m1guelperez commented Mar 8, 2022 • edited Loading

wesleywiser commented Mar 8, 2022

Mark-Simulacrum commented Mar 11, 2022

bors commented Mar 11, 2022

m1guelperez commented Mar 11, 2022 • edited Loading

m1guelperez commented Jan 24, 2022 •

edited by Mark-Simulacrum

Loading

hkratz left a comment •

edited

Loading

m1guelperez commented Jan 25, 2022 •

edited

Loading

the8472 commented Jan 25, 2022 •

edited

Loading

m1guelperez commented Jan 25, 2022 •

edited

Loading

m1guelperez commented Mar 3, 2022 •

edited

Loading

m1guelperez commented Mar 7, 2022 •

edited

Loading

m1guelperez commented Mar 8, 2022 •

edited

Loading

m1guelperez commented Mar 11, 2022 •

edited

Loading