fix(core): run-commands cwd issue

[JakubKoralewski]

setting cwd option for run-commands would result in an error
if the command was run from somewhere that is not a workspace root

Current Behavior

Running a run-commands executor command from outside the root with the cwd option set is not working.

https://stackoverflow.com/questions/18894433/nodejs-child-process-working-directory
nodejs/node#11520

This is the error:

$ nx db-migrate

> nx run app:db-migrate
ERROR: Something went wrong in @nrwl/run-commands - spawnSync C:\Windows\system32\cmd.exe ENOENT

Expected Behavior

I tried to change the cwd to the workspace root if the joined path of process.cwd() and the provided cwd did not exist.

I'm having issues actually testing whether this works, with line endings on WSL and the local release giving me errors. Just not a nice experience for me. I'd appreciate if someone was able to take over this PR and properly test this.

EDIT: Additionally, just remembering to call all the commands from the workspace root is not a valid fix/workaround, because if I want to nest my commands, where both have the cwd option set then the options.commands command which calls the other nx command (nx run app:other-command) will crash, because I think the first command goes to (pseudojs) ${root}${firstCommand.cwd} and the second call attempts to do something like this ${root}${firstCommand.cwd}${secondCommand.cwd} and there's an error.

So the current workaround is:

1. Remove any nesting in your workspace.json.
2. Call commands only from root.

[nx-cloud]

Nx Cloud Report

CI ran the following commands for commit 6ed2d6a. Click to see the status, the terminal output, and the build insights.

Status	Command	Start Time
	nx run-many --target=build --all --parallel	2/7/2021, 12:51:10 PM
	nx run-many --target=e2e --projects=e2e-angular	2/7/2021, 12:53:12 PM
	nx run-many --target=e2e --projects=e2e-cypress,e2e-jest	2/7/2021, 12:52:16 PM
	nx run-many --target=e2e --projects=e2e-next,e2e-gatsby	2/7/2021, 1:03:01 PM

---

Sent with 💌 from NxCloud.

[JakubKoralewski]

I tried to change the cwd to the workspace root if the joined path of process.cwd() and the provided cwd did not exist.

On further thought, this doesn't make sense. the checkCwd function should probably just merge the context.root with schema.cwd.

[vsavkin]

Thank you for submitting the PR.

How about the following rules:

1. CWD isn't set, then CWD isn't set on the execSync
2. If CWD is set and it is not absolute, then we join context.root and the provided CWD
3. If CWD is set and it is absolute, we just pass the CWD

Basically, your current working directly when invoking a command only matters for 1. If you want it not to matter, you can set it to ".", and it will always be the workspace root.

What do you think?

[JakubKoralewski]

Thank you as well for including me in the PR process :) I'm not too familiar with the codebase so I guess by you not pointing something is clearly wrong means my gamble on this file being the source of the run-commands executor implementation was correct. Also I hope it's fine how I just added another parameter to the default exported function in run-commands.impl.ts, cause I saw it in another executor implementation and wasn't able to test.

What do you think?

I agree with the rules you created. Even more the rules 1, 3 are already established by the current implementation of the checkCwd function (if it is true that if a cwd is not given then it is undefined) as it passes the undefined to execSync and exec (and assuming undefined means cwd isn't set on exec or execSync). I didn't think through the implementation I provided in this PR for the 2nd rule but I totally agree with it.

Not setting cwd on exec means that it is process.cwd() which I guess is the workspace root anyway?

If you want it not to matter, you can set it to ".", and it will always be the workspace root.

On the one hand it's perfectly understandable, but also the way you put it is a bit confusing to me (as a user) because both an empty cwd "", ".", "./", and not setting the cwd all mean the "commands" are being run from the workspace root anyway? So I agree with what you say, but if I didn't care about the cwd then I just wouldn't set it though. But as I read through the docs it's actually not specified?

If you answer that the empty cwd property in run-commands signifies that the command's cwd will be whatever the current working directory is, this could introduce problems (as I mentioned earlier) with nesting commands. I think it would be nice to document what's the default behaviour of not specifying a cwd.

I think same goes for "", "." and "./" that it could both mean relative to cwd or relative to workspace root.

I am personally of the opinion to have it be relative to workspace root, what's your opinion?

[vsavkin]

To be more concrete, these are the two options I see:

What I outlined above:

function calculateCwd(cwd: string | undefined, context: ExecutorContext): string | undefined {
  if(!cwd) return undefined;
  if(path.isAbsolute(cwd)) return cwd;
  return path.join(context.root, cwd);
}

This option sets cwd to root when not specified. No issues with nesting.

function calculateCwd(cwd: string | undefined, context: ExecutorContext): string {
  if(!cwd) return context.root;
  if(path.isAbsolute(cwd)) return cwd;
  return path.join(context.root, cwd);
}

I personally prefer Option 2 cause it's more predictable, but it's not how it works right now. Since current behaviour is under-specified, we could treat it as a bug fix.

As a side note, we also need to update run-script.impl.ts to set cwd like this:

cwd: path.join(content.root, context.workspace.projects[context.projectName].root)

As a second side note :), one way to test it if e2e don't run for it for some reason is to run: nx build workspace and then find the compiled JS file, copy its content and paste them into your workspace (find the same file in node_modules).

[vsavkin]

@JakubKoralewski let me know if you need any help

[leosvelperez]

@JakubKoralewski thanks for sending the PR!
Closing as it was fixed in #5119.

[github-actions]

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.