fix: use real path for module root dir (#6524)

[grimly]

Resolves #6524

Intro

I was not able to verify the fix with the mentioned pd command. I applied the same changes to this repository as I did on my local installation of pnpm for the issue to be fixed.

I might not have fixed the original issue so I'll describe here the issue I faced and how I fixed it.

How to reproduce constantly the issue

• This issue only concerns systems that are NOT case sensitive in their file path (basically Windows).
• Place yourself in a folder where to create a test project, make sure the parent folder is either not existent or not important
• Run the following commands (I'm using gitbash but equivalents in powershell or cmd behave the same) :

  [ ! -e parent ] || rm -rf parent
  mkdir -p parent/workspaces
  (
    cd Parent # Bad casing is INTENTIONAL and part of why the bug happen
    echo -e '{\n  "name": "parent",\n  "private": true\n}' > package.json
    echo -e 'packages:\n  - "workspaces/*"' > pnpm-workspace.yaml
    pnpm add -P rxjs
  )

My analysis

At some point in a pnpm add command, There is a mismatch when fetching the manifest of an importer because their rootDir uses the bad Parent folder name instead of the parent real folder name.

I threw some logs right before that call ( console.log([Object.keys(manifestsByPath), rootDir]) ) and I got the following log :

[
  [ 'E:\\code\\parent' ],
  'E:\\code\\Parent'
]

I saw that you fetch the real path earlier in the function to check either or not the folder is part of the workspaces so I added the same and it worked !
Of course my local change was different as I acted on the compiled output and here is what I did :

(Change is at line 190518 of pnpm.cjs)

          const isFromWorkspace = is_subdir_1.default.bind(null, calculatedRepositoryRoot);
          importers = await (0, p_filter_1.default)(importers, async ({ rootDir }) => isFromWorkspace(await fs_1.promises.realpath(rootDir)));
+         importers = await Promise.all(importers.map(async ({ rootDir, ...rest }) => ({ rootDir: await fs_1.promises.realpath(rootDir), ...rest })));
          if (importers.length === 0)
            return true;

Notes

If you have better knowledge of where this rootDir is from, you may want to update to the realpath earlier and save other unidentified bugs related to it.

[welcome]

💖 Thanks for opening this pull request! 💖
Please be patient and we will get back to you as soon as we can.

[zkochan]

I am not sure why this is needed. We run fast-glob on the workspace directory. And workspace directory should already be resolved to real path:

pnpm/workspace/find-workspace-dir/src/index.ts

Line 13 in c0182b4

: await findUp([WORKSPACE_MANIFEST_FILENAME, 'pnpm-workspace.yml'], { cwd: await getRealPath(cwd) })

[grimly]

So I'll go deeper and find the source.

Here is my analysis on where the faulty values comes from :

• importers[].rootDir uses the keys of opts.selectedProjectsGraph
  I found also when adding logs also opts.allProjectsGraph which has different keys (actually the right ones).
• Following up the stacktrace, opts is provided by installDeps
• opts.selectedProjectsGraph is generated here. At this point opts.selectedProjectsGraph is undefined, allProjects is an array of objects with the correct paths in their dir property, and opts.dir has the faulty root directory.
• selectProjectByDir uses its searchedDir parameter without any change. Now we have to search for the opts.dir faulty value.
• Here the stacktrace forget things ... installDeps is called here but nothing is done to opts.dir
• handler is the handler for the install command in the test scenario above. It is called and awaited here.. At this point we look for config.dir.
• config is generated by a getConfig function.
• A quick tour to getConfig from cli-utils which is more or less a wrapper ...
• We arrive at the real getConfig
• Here again a new name for our target, we now look for pnpmConfig.dir
• Our target is declared differently depending if it is a global command or if it is local
• On the local path, the cwd is declared here

Somehow my scenario above, using this current PR don't catch we're at the parent level.
Fixing the issue at the cwd level, pnpm shows the ERR_PNPM_ADDING_TO_ROOT error which is expected.

[zkochan]

You can make this operation synchronous as we don't run anything concurrently at this point.

[grimly]

I know for a fact you are already asynchronous really early in this function.
Also you do not start additional tasks so it should not be a problem.

[zkochan]

That can be changed too. But not in this PR. Sync fs operations in Node.js are faster unless you can run them in parallel to something else.

[welcome]

Congrats on merging your first pull request! 🎉🎉🎉