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

feat: windows support #1184

Merged
merged 36 commits into from
Jul 7, 2020
Merged

feat: windows support #1184

merged 36 commits into from
Jul 7, 2020

Conversation

Weakky
Copy link
Collaborator

@Weakky Weakky commented Jul 6, 2020

closes #549
closes #838
closes #965
closes #538

TODO

  • tests

src/lib/process/index.ts Show resolved Hide resolved
src/lib/e2e-testing.ts Show resolved Hide resolved
src/lib/glocal/detect-exec-layout.spec.ts Show resolved Hide resolved
src/lib/glocal/detect-exec-layout.ts Outdated Show resolved Hide resolved
src/lib/glocal/detect-exec-layout.ts Outdated Show resolved Hide resolved
path.js Outdated Show resolved Hide resolved
src/lib/glocal/detect-exec-layout.ts Show resolved Hide resolved
@@ -43,7 +43,7 @@ export function setup({ run, toolName, depName, filename }: SetupInput): void {
// use envar to boost perf, skip costly detection work
if (!process.env.GLOBAL_LOCAL_HANDOFF) {
log.trace('execLayout start')
const execLayout = detectExecLayout({ depName })
const execLayout = detectExecLayout({ depName, scriptPath: filename })
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this mean that we're no longer using the scriptPath default of process.argv[0]? Why? Remove the default if no longer needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah so, we haven't talked about it yet, but it turns out that on Windows, argv[1] is always an absolute path to the js file and never the .bin/ path. Therefore, in order to "normalize" paths betweeen the platforms, I've decided to always pass the __filename.
This is also the reason why I had to implement the logic to resolve the script path from the package.json bin property. Since we never get the information of the .bin/ executable on windows, and since argv[1] is also an absolute path to the script, the only way to compare the executed script path with the local script path is to resolve the path of the script from the package.json.

Copy link
Member

Choose a reason for hiding this comment

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

is always an absolute path to the js file and never the .bin/ path.

As was the case on macOS (or Linux, one of the two, can't remember). That's what this was dealing with for example.

Since we never get the information of the .bin/ executable on windows

What do we need that info for, though?

The ultimate goal has always been to compare the executed scripts of current process vs current project.

I had previously worked with .bin and symlinks because that's what argv[1] pointed to before. I suspect that the symlink was good for performance because of less IO to trace the project tool path.

What I'm hearing now is that in Windows, this work is essentially already done for us. It sounds like more reason, not less, to use argv[1]?

src/lib/glocal/glocal.ts Show resolved Hide resolved
src/lib/utils/index.ts Outdated Show resolved Hide resolved
@Weakky Weakky requested a review from jasonkuhrt July 7, 2020 09:30
@@ -178,8 +185,10 @@ export function createE2EContext(config: Config) {
export function spawn(command: string, args: string[], opts: IPtyForkOptions): ConnectableObservable<string> {
const nodePty = requireNodePty()
const subject = new Subject<string>()
// On windows, node-pty needs an absolute path to the executable. `which` is used to find that path.
Copy link
Member

@jasonkuhrt jasonkuhrt Jul 7, 2020

Choose a reason for hiding this comment

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

Is this an alternative solution to supplying shell? Is the reason we don't supply shell here that node-pty is missing that from its API?

Copy link
Member

@jasonkuhrt jasonkuhrt left a comment

Choose a reason for hiding this comment

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

🚢 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants