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: normalize Deno.execPath #2598

Merged
merged 11 commits into from Jun 30, 2019

Conversation

2 participants
@bartlomieju
Copy link
Contributor

commented Jun 28, 2019

Fixes #1798

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

I'm wondering if using Path.components would be better solution for Path normalization than using Url parser. Components indeed removes . from paths (a/./b -> a/b) but leaves .. because they may be symlinks.

@ry
Copy link
Collaborator

left a comment

Cool - looks good. Could you also add this test:

diff --git a/js/os_test.ts b/js/os_test.ts
index 766cd1c3..350a3d4b 100644
--- a/js/os_test.ts
+++ b/js/os_test.ts
@@ -42,3 +42,8 @@ test(function osIsTTYSmoke(): void {
 test(function homeDir(): void {
   assertNotEquals(Deno.homeDir(), "");
 });
+
+test(function execPathNormalized(): void {
+  // See https://github.com/denoland/deno/issues/1798
+  assert(!Deno.execPath.includes("/./"));
+});
Show resolved Hide resolved cli/ops.rs Outdated
@ry

This comment has been minimized.

Copy link
Collaborator

commented Jun 28, 2019

Hmm actually the test that I provided doesn't fail on master...

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2019

Hmm actually the test that I provided doesn't fail on master...

That's because we create absolute path to Deno executable in test suite.

bartlomieju added some commits Jun 28, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:fix-execPath_unneeded_segments branch from 0433128 to 35b0259 Jun 28, 2019

bartlomieju added some commits Jun 28, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:fix-execPath_unneeded_segments branch from e4fbded to dacb63c Jun 29, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:fix-execPath_unneeded_segments branch 2 times, most recently from 8123873 to 2986552 Jun 29, 2019

@bartlomieju bartlomieju force-pushed the bartlomieju:fix-execPath_unneeded_segments branch from 2986552 to b742161 Jun 29, 2019

@bartlomieju

This comment has been minimized.

Copy link
Contributor Author

commented Jun 30, 2019

@ry this PR should be landable now


if (Deno.platform.os !== "win") {
main();
}

This comment has been minimized.

Copy link
@ry

ry Jun 30, 2019

Collaborator

This test is pretty complicated, and I don't like that it's not runnable outside the test.py framework.. (I tried running it manually on my computer, and it failed)

This patch is a fairly minor change. I would say let's just land it without tests...

@ry

ry approved these changes Jun 30, 2019

Copy link
Collaborator

left a comment

LGTm - sorry for the extra work on the test case.

@ry ry merged commit 9d18f97 into denoland:master Jun 30, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.