-
Notifications
You must be signed in to change notification settings - Fork 29.8k
Use platform appropriate filepaths #44221
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
Conversation
| final PoolResource resource = await pool.request(); | ||
| try { | ||
| final File file = fs.file(fs.path.join(outputDirectory.path, entry.key)); | ||
| final File file = fs.file(outputDirectory.uri.resolve(entry.key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this fix needed anywhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated bundle.dart
| await buildSystem.build(const CopyAssets(), environment); | ||
|
|
||
| expect(fs.file(fs.path.join(environment.buildDir.path, 'flutter_assets', 'assets/foo/bar.png')).existsSync(), true); | ||
| expect(fs.file(fs.path.join(environment.buildDir.path, 'flutter_assets', 'assets', 'foo', 'bar.png')).existsSync(), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it important for the test that you get different instances for the two repetitions of:
fs.file(fs.path.join(environment.buildDir.path, 'flutter_assets', 'assets', 'foo', 'bar.png') ?
If so, please add a comment. If not, then stash either the File or the String for the path in a local.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
|
Fixes #35293 |
|
This also causes #46163. I think we want ot keep all these as URI encoded because the framework sends over the file names as URI encoded. If anything we should make sure we copy only the URI encoded file, and not the unencoded file. |
|
It should be safe to revert this PR. That revert should include tests of the currently failing to load assets, lest we repeat this in 10-12 months |
Description
Works on my computer?