jsvu icon indicating copy to clipboard operation
jsvu copied to clipboard
verified publisher icon GoogleChromeLabs

Incorrect path being made in .cmd wrapper on windows.

Open rwaldron opened this issue 6 years ago • 5 comments

Following the execution path:

https://github.com/GoogleChromeLabs/jsvu/blob/853836d94d207f6c8f487f272121b0bd4a5f7b72/shared/installer.js#L46 https://github.com/GoogleChromeLabs/jsvu/blob/853836d94d207f6c8f487f272121b0bd4a5f7b72/shared/installer.js#L131-L132 https://github.com/GoogleChromeLabs/jsvu/blob/853836d94d207f6c8f487f272121b0bd4a5f7b72/shared/installer.js#L134

Which results in this:

@echo off
"%~dp0engines/v8\v8.exe" --natives_blob="%~dp0engines/v8\natives_blob.bin" --snapshot_blob="%~dp0engines/v8\snapshot_blob.bin" %*

rwaldron avatar Apr 12 '19 15:04 rwaldron

What's the expected result? What's broken exactly?

mathiasbynens avatar Apr 12 '19 16:04 mathiasbynens

Aren't these invalid windows paths?

  • engines/v8\v8.exe
  • engines/v8\natives_blob.bin
  • engines/v8\snapshot_blob.bin

This portion: engines/v8, which gets made in this.targetRelPath = `engines/${engine}`; is never passed through a path.normalize().

rwaldron avatar Apr 12 '19 18:04 rwaldron

cmd.exe accepts both \ and /. We could change the remaining / into \ on Windows but it would be an unobservable change 🤷🏼‍♂️

mathiasbynens avatar Apr 12 '19 20:04 mathiasbynens

To be clear, I'd definitely accept a patch that uses path.join instead of hardcoding the / in shared/installer.js. It's just not a priority IMHO since it doesn't change behavior.

mathiasbynens avatar Apr 24 '19 12:04 mathiasbynens