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

jsc.exe --module-file should understand Windows paths #1449

Conversation

rkirsling
Copy link
Member

@rkirsling rkirsling commented Jun 10, 2022

47fde25

jsc.exe --module-file should understand Windows paths
https://bugs.webkit.org/show_bug.cgi?id=241518

Reviewed by Yusuke Suzuki.

jsc.cpp's module loader was written without any accommodation for Windows, so:
1. On Windows, recognize C:\foo as an absolute path and .\foo and ..\foo as dotted relative paths (allowing '/' too).
2. On all platforms, stop misusing the URL(base, relative) constructor. This isn't the way to add file:/// to an abspath.

This ensures that module tests are able to run well on Windows.

* Source/JavaScriptCore/jsc.cpp:
(isAbsolutePath): Added.
(isDottedRelativePath): Added.
(absoluteFileURL): Renamed from `absolutePath`.
(GlobalObject::moduleLoaderImportModule):
(GlobalObject::moduleLoaderResolve):
(JSC_DEFINE_HOST_FUNCTION):
(computeFilePath):
(runWithOptions):

Canonical link: https://commits.webkit.org/251514@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295509 268f45cc-cd09-0410-ab3c-d52691b4dbfc

@rkirsling rkirsling self-assigned this Jun 10, 2022
@rkirsling rkirsling added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 10, 2022
@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jun 11, 2022
@rkirsling rkirsling removed merging-blocked Applied to prevent a change from being merged JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 11, 2022
@rkirsling rkirsling force-pushed the eng/jsc-exe---module-file-should-understand-Windows-paths branch from 4a44476 to 4fee200 Compare June 11, 2022 01:44
@rkirsling rkirsling added JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. WebKit Nightly Build labels Jun 11, 2022
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

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

r=me

// Just look for local drives like C:\.
return path.length() > 2 && isASCIIAlpha(path[0]) && path[1] == ':' && (path[2] == '\\' || path[2] == '/');
#else
return path.startsWith('/');
Copy link
Member

Choose a reason for hiding this comment

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

Since it is used for module specifier too, I think / root path should be supported too in Windows. And in that case, we should make a path from the current path's drive.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, are you sure? It seems like other engines still treat that as a relative path on Windows:

test.js

import result from '/GitHub/WebKit/test-module.js';
print(result);

Result:

PS C:\GitHub\WebKit> ~\.jsvu\v8.cmd --module test.js
undefined:0: Error: d8: Error reading  module from C:/GitHub/WebKit//GitHub/WebKit/test-module.js
    imported by C:/GitHub/WebKit/test.js

PS C:\GitHub\WebKit> ~\.jsvu\sm.cmd --module test.js
Error: can't open C:\GitHub\WebKit\\GitHub\WebKit\test-module.js: No such file or directory
Stack:

@rkirsling rkirsling force-pushed the eng/jsc-exe---module-file-should-understand-Windows-paths branch from 4fee200 to efa526c Compare June 14, 2022 00:51
@rkirsling rkirsling added the merge-queue Applied to send a pull request to merge-queue label Jun 14, 2022
https://bugs.webkit.org/show_bug.cgi?id=241518

Reviewed by Yusuke Suzuki.

jsc.cpp's module loader was written without any accommodation for Windows, so:
1. On Windows, recognize C:\foo as an absolute path and .\foo and ..\foo as dotted relative paths (allowing '/' too).
2. On all platforms, stop misusing the URL(base, relative) constructor. This isn't the way to add file:/// to an abspath.

This ensures that module tests are able to run well on Windows.

* Source/JavaScriptCore/jsc.cpp:
(isAbsolutePath): Added.
(isDottedRelativePath): Added.
(absoluteFileURL): Renamed from `absolutePath`.
(GlobalObject::moduleLoaderImportModule):
(GlobalObject::moduleLoaderResolve):
(JSC_DEFINE_HOST_FUNCTION):
(computeFilePath):
(runWithOptions):

Canonical link: https://commits.webkit.org/251514@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@295509 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@webkit-early-warning-system webkit-early-warning-system merged commit 47fde25 into WebKit:main Jun 14, 2022
@webkit-early-warning-system webkit-early-warning-system force-pushed the eng/jsc-exe---module-file-should-understand-Windows-paths branch from efa526c to 47fde25 Compare June 14, 2022 01:00
@webkit-early-warning-system
Copy link
Collaborator

Committed r295509 (251514@main): https://commits.webkit.org/251514@main

Reviewed commits have been landed. Closing PR #1449 and removing active labels.

@webkit-early-warning-system webkit-early-warning-system removed the merge-queue Applied to send a pull request to merge-queue label Jun 14, 2022
@rkirsling rkirsling deleted the eng/jsc-exe---module-file-should-understand-Windows-paths branch June 14, 2022 01:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
3 participants