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

fix: cache paths on Windows are broken #2760

Merged
merged 15 commits into from Aug 11, 2019

Conversation

@bartlomieju
Copy link
Contributor

commented Aug 10, 2019

Cache paths are not properly generated on Windows.

It's broken since v0.13.0.

Ref:
denoland/deno_std#552
denoland/deno_std#556

Example:

file:///C:/deno/js/unit_test_runner.ts
// for above URL this cache path should be generated 
file\c\deno\js\unit_test_runner.ts
// this is produced (probably)
C:/deno/js/unit_test_runner.ts
bartlomieju added 3 commits Aug 10, 2019
fix

@bartlomieju bartlomieju changed the title fix: cache path for Windows fix: cache paths on Windows are broken Aug 10, 2019

bartlomieju added 9 commits Aug 10, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:fix-cache_path_for_windows branch from cdc5122 to c9e1853 Aug 11, 2019

bartlomieju added 2 commits Aug 11, 2019
@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

@piscisaureus I think I managed to make it work. We're gonna need to do a release with this change to fix CI in standard lib

let disk = (disk_byte as char).to_string();
out.push(disk);
}
_ => {}

This comment has been minimized.

Copy link
@piscisaureus

piscisaureus Aug 11, 2019

Collaborator

Add an unreachable() here. Let's not silently discard part components.

This comment has been minimized.

Copy link
@bartlomieju

bartlomieju Aug 11, 2019

Author Contributor
@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

This fixes a very acute problem, so I'm all for landing this.
However, we have similar problems with other path components, which can also contain illegal characters (e.g. ? and &).

Tangentially related, there's also a correctness issue w.r.t. case sensitivity. In URLs the path and query component are case sensitive, while the protocol and host are not. When deno_dir lives on a case insensitive file system it won't be able to handle that properly.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

However, we have similar problems with other path components, which can also contain illegal characters (e.g. ? and &).

Haven't thought about it, but I believe Url won't return them from path_segments method. That inherently means that http://example.com/a.js?foo would be cached as http/example.com/a.js.

Tangentially related, there's also a correctness issue w.r.t. case sensitivity. In URLs the path and query component are case sensitive, while the protocol and host are not. When deno_dir lives on a case insensitive file system it won't be able to handle that properly.

Agreed, any ideas how to tackle that? Cast all protocols and hostnames to lowercase?

@piscisaureus

This comment has been minimized.

Copy link
Collaborator

commented Aug 11, 2019

Haven't thought about it, but I believe Url won't return them from path_segments method. That inherently means that http://example.com/a.js?foo would be cached as http/example.com/a.js.

That doesn't seem right either. Only the part after # can be safely discarded.

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

That doesn't seem right either. Only the part after # can be safely discarded.

Agreed, shall I open a new issue with two described edge cases and we can land this patch?

@piscisaureus piscisaureus merged commit 54982e9 into denoland:master Aug 11, 2019

3 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla Contributor License Agreement is signed.
Details

@bartlomieju bartlomieju deleted the bartlomieju:fix-cache_path_for_windows branch Aug 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants
You can’t perform that action at this time.