Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Fix Windows file checks of unicode paths #16105

Merged

Conversation

stuartmorgan-g
Copy link
Contributor

FileExists is currently implemented using stat on windows, which
interprets the path in an environment-determined way rather than as
unicode. This replaces that implementation with wide-string-based
Win32 calls that are guaranteed to use unicode.

This also changes the semantics of FileExists on Windows, since the
current implementation is inconsistent with the POSIX version; the latter
will return true for a directory that exists, whereas the Windows
implementation returned true only for a regular (non-directory, non-link)
file. The function is not documented, so it's not clear which was intended,
so I assumed the POSIX version was authoritative.

Fixes flutter/flutter#49372

@stuartmorgan-g stuartmorgan-g added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 29, 2020
@fluttergithubbot
Copy link
Contributor

This pull request is not suitable for automatic merging in its current state.

  • The status or check suite Windows Android AOT Engine has failed. Please fix the issues identified (or deflake) before re-applying this label.

@fluttergithubbot fluttergithubbot removed the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 29, 2020
return false;
return S_ISREG(buf.st_mode);
}
return !(attributes &
Copy link
Member

Choose a reason for hiding this comment

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

Why not follow symlinks?

Copy link
Contributor Author

@stuartmorgan-g stuartmorgan-g Jan 29, 2020

Choose a reason for hiding this comment

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

Won't S_ISREG be false for symlinks?

(As noted in the PR description, it's hard to implement these functions when there's absolutely no documentation about what they are intended to mean. Or tests enforcing any specific semantics...)

@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

FileExists is currently implemeented using stat on windows, which
interprets the path in an environment-determined way rather than as
unicode. This replaces that implementation with wide-string-based
Win32 calls that are guaranteed to use unicode.

Fixes flutter#49372
@stuartmorgan-g stuartmorgan-g force-pushed the windows-unicode-file-exists branch from e2cd21b to 69ab0f5 Compare January 30, 2020 19:17
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

1 similar comment
@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@stuartmorgan-g stuartmorgan-g added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Jan 30, 2020
@fluttergithubbot fluttergithubbot merged commit 4655295 into flutter:master Jan 30, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 31, 2020
engine-flutter-autoroll added a commit to flutter/flutter that referenced this pull request Jan 31, 2020
flutter/engine@74d07b5...804dca6

git log 74d07b5..804dca6 --first-parent --oneline
2020-01-31 [email protected] Use bundled Roboto in all tests (flutter/engine#16218)
2020-01-31 [email protected] Revert "Migrate flutter_runner from flutter_runner::{Thread,Loop} to fml::{Thread,MessageLoop} (#15118)" (flutter/engine#16277)
2020-01-31 [email protected] Roll src/third_party/skia 36c0521d57de..6305b2f8342a (8 commits) (flutter/engine#16272)
2020-01-31 [email protected] Revert "[web] Correct getPositionForOffset for multi-line paragraphs (#16206)" (flutter/engine#16268)
2020-01-30 [email protected] Fix Windows file checks of unicode paths (flutter/engine#16105)
2020-01-30 [email protected] [fuchsia] Fix the import for dart_api.h (flutter/engine#16269)
2020-01-30 [email protected] [fuchsia] SceneHostBindings are no longer thread locals (flutter/engine#16262)
2020-01-30 [email protected] Pass through invoker.resources in fuchsia_test_archive (flutter/engine#16265)
2020-01-30 [email protected] Notify PlatformViewsController within FlutterEngine when a hot restart occurs. (#48518) (flutter/engine#16230)
2020-01-30 [email protected] Roll src/third_party/dart fc3af737c759..162d6c5634a0 (209 commits) (flutter/engine#16261)
2020-01-30 [email protected] Roll src/third_party/skia d1be5d64f8a7..36c0521d57de (13 commits) (flutter/engine#16260)
2020-01-30 [email protected] [web] Correct getPositionForOffset for multi-line paragraphs (flutter/engine#16206)
2020-01-30 [email protected] Roll fuchsia/sdk/core/linux-amd64 from -mGIA... to 93K0d... (flutter/engine#16257)


If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC [email protected] on the revert to ensure that a human
is aware of the problem.

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+/master/autoroll/README.md
NoamDev pushed a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
NoamDev added a commit to NoamDev/engine that referenced this pull request Feb 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Windows] Path handling does not support UTF-8 properly
5 participants