[BUG] Running NX command is not working with pnpm

robinpellegrims commented 2 years ago

I'm submitting a...


[X] Regression (a behavior that used to work and stopped working in a new release)
[ ] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request
[ ] Other... Please describe:

Current behavior

I'm using pnpm as package manager and it always worked perfectly in the past. Currently e-square-io/nx-distributed-task fails with the following error:

🏃 Running NX target
  /home/runner/setup-pnpm/node_modules/.bin/pnpm exec -p @nrwl/cli nx run-many --projects=markdown --skip-nx-cache=false --target=test --parallel=3
  undefined
   ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  not found: -p
Error: Error: The process '/home/runner/setup-pnpm/node_modules/.bin/pnpm' failed with exit code 1

The command also fails locally, as "pnpm exec" doesn't have a flag -p. (https://pnpm.io/cli/exec)

// this fails
$ pnpm exec -p @nrwl/cli nx run-many ---projects=markdown --target=test --parallel=3
 ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL  not found: -p

// this works
$ pnpm exec nx run-many --projects=markdown --target=test --parallel=3

 >  NX   Running target test for 1 project

Last known version that worked without any issues: v2.2.6. In this version, the run-many command was different: /home/runner/setup-pnpm/node_modules/.bin/nx run-many --projects=markdown --target=test --parallel --maxParallel=3

Expected behavior

It should continue to work with pnpm.

Minimal reproduction of the problem with instructions

Create a github action on a repository and install pnpm:

    - name: Install and execute pnpm
      uses: pnpm/action-setup@v2.2.1
      with:
        version: 6.32.3
        run_install: |
          - args: [--frozen-lockfile, --prefer-offline]

Use the latest version of e-square-io/nx-distributed-task to execute the tasks.

      - name: Execute
        uses: e-square-io/nx-distributed-task@v2.3.4

Environment

- Nx version: 13.8.7
- Platform:  Linux

ronnetzer commented 2 years ago

Hi @robinpellegrims

Thank you for reporting the issue, would you like to contribute and fix it? It should be pretty easy, the changes are only in nx-distributed-file/src/app/nx.ts file in nxCommand method

...
const wrapper = exec
  .withCommand(`${command} -p @nrwl/cli nx ${nxCommand}`)
...

the extra -p @nrwl/cli arguments should move to be part of the command if the package manager is npm.

It should be checked what is the equivalent for each package manager

extra info: the actions uses getPackageManagerCommand from NX to retrieve the commands for the relevant package manager. for npm its npx for yarn its yarn and for pnpm its pnpx and above v6.13 its pnpm exec Apparently NX is not checking if yarn 2 is being used which doesn't have the list command (it uses a different info command) so we'll need to add a check for yarn's version and to adjust the command accordingly.

Also, what about asserting that NX is installed? does this step works as expected? I assume so as your error is after this step but just to make sure.

robinpellegrims commented 2 years ago

Sure, I'll take a look.

The assertNxInstalled did work because it was hardcoded to use npm instead of pnpm, but it seems you fixed that today to use the installed package manager instead.

Also, it seems that the actual equivalent of npx -p @nrwl/cli is pnpm --package @nrwl/cli dlx. Should we go down that road or stay close to what nx is returning in getPackageManagerCommand ?

ronnetzer commented 2 years ago

@robinpellegrims let's stay close to what NX returns

e-square-io / nx-github-actions