Issues with beta version using relative file paths

[nvuillam]

          Thanks for the heads up, @nvuillam. I tried the `documentation`, `dotnet`, `javascript`, and `python` flavors of the beta version on all of our repositories and encountered three issues. I am listing them all here for now since the second and third seem like they could potentially be related, but I am happy to file issues about these when I have more time tomorrow. The third issue is a preexisting issue that was made more visible by #2455, because previously ESLint silently didn't run at all locally.

1. CSharpier is broken; I am curious whether anyone else encounters this issue:

   ❌ Linted [CSHARP] files with [csharpier]: Found 1 error(s) - (1.18s)
   --Error detail:
   Run "dotnet tool restore" to make the "dotnet-csharpier" command available.
   
   Unable to get number of errors with regex_number and Issues found: ([0-9]+) in .* files
   

2. CSpell is broken when importing dictionaries that aren't bundled:

   ❌ Linted [SPELL] files with [cspell]: Found 1 error(s) - (18.84s)
   --Error detail:
   Configuration Error: Failed to read config file: "/tmp/lint/@cspell/dict-medicalterms/cspell-ext.json"
   CSpell: Files checked: 0, Issues found: 0 in 0 files
   
   ❌ Error(s) have been found during linting
   

   Our cspell.config.yaml contains:

   import:
     - "@cspell/dict-medicalterms/cspell-ext.json"

   Our .mega-linter.yaml contains:

   SPELL_CSPELL_CONFIG_FILE: LINTER_DEFAULT
   SPELL_CSPELL_PRE_COMMANDS:
     - command: npm install @cspell/[email protected]

3. In our Yarn TypeScript projects, there is one error per file of the following form:

   ❌ Linted [TYPESCRIPT] files with [eslint]
   --Error detail:
   
   /tmp/lint/some_file.ts
     0:0  error  Parsing error: File '@tsconfig/node18-strictest-esm/tsconfig.json' not found
   

   Our tsconfig.json contains:

   {
     "extends": "@tsconfig/node18-strictest-esm/tsconfig.json"
   }

   Our package.json contains:

   {
     "devDependencies": {
       "@tsconfig/node18-strictest-esm": "1.0.1"
     }
   }

   Our .eslintrc.yaml is:

   root: true
   extends:
     - eslint:recommended
     - plugin:@typescript-eslint/recommended
     - prettier
   parser: "@typescript-eslint/parser"
   parserOptions:
     project:
       - tsconfig.json
   plugins:
     - "@typescript-eslint"
   env:
     node: true

Originally posted by @Kurt-von-Laven in #1877 (comment)

[nvuillam]

thanks for reporting @Kurt-von-Laven ... let's find out what can be wrong ^^

Own MegaLinter linting is using beta, and for now it's ok -> https://github.com/oxsecurity/megalinter/actions/runs/4752536246/jobs/8443012531

[nvuillam]

It seems for the moment the only major error is npm package dependencies ... maybe the issue with csharpier is just a build issue :/

[Kurt-von-Laven]

If I understand correctly, the CSharpier issue means MegaLinter will break for anyone with C# files who didn't disable CSharpier.

[Kurt-von-Laven]

I think the third one may be related to typescript-eslint/typescript-eslint#1592. It's also similar to typescript-eslint/typescript-eslint#5223.

[Kurt-von-Laven]

More specifically, the third issue is related to the way we install our Node.js dependencies, namely to /node-deps. ESLint is leaning on TypeScript to find the @tsconfig/node18-strictest-esm package that gets installed to /node-deps via a pre-command, but TypeScript neither honors NODE_PATH nor has its own configuration option to search a directory other than node_modules when resolving packages in extends. I was able to confirm this theory in a minimal reproduction with a small Dockerfile, and the following workaround works for us:

POST_COMMANDS:
  - command: rm -r node_modules
    cwd: workspace
TYPESCRIPT_ES_PRE_COMMANDS:
  - command: npm install @tsconfig/[email protected]
  - command: cp --recursive /node-deps/node_modules/ node_modules
    cwd: workspace

This is pretty complex, so I wonder if anyone has any ideas that would make the user experience friendlier for TypeScript + Yarn PnP projects.

[nvuillam]

So... maybe we should not force the cd node_deps when we have a npm install instruction ?

[Kurt-von-Laven]

I pondered that as well, but I'm not so sure everyone will consider that a friendlier developer experience. On the one hand, running npm install @tsconfig/[email protected] in the workspace would install the module directly in the place TypeScript will ultimately look for it. On the other, it will also install everything in your package.json into the node_modules directory in your workspace that you didn't think you were using and create package-lock.json. I haven't checked pnpm, but neither npm nor Yarn offer a means by which to install only a single package to node_modules in their CLIs. Installing all packages could be a considerable slowdown for projects with large dependency trees, particularly since they may not think to cache npm dependencies in CI if they aren't using npm directly. In addition, if they didn't think to clean node_modules with a post-command, Yarn PnP (or pnpm) projects would end up with two copies of all of their the dependencies, one of which (the one in node_modules) is probably owned by root and isn't used by Yarn/pnpm. This can create confusion, particularly since the versions most likely wouldn't stay in sync as they have separate lock files managed by different tools. At the end of the day, I think this issue demonstrates the subtle pitfalls of running Node.js modules via a different package manager than the one a project is using. One thing that would potentially make this all better would be supporting arrays in the *_CLI_EXECUTABLE config option, which I suspect might be a one-line change. That would allow users to configure MegaLinter to run ESLint via Yarn, allowing TypeScript to find ESLint installed via Yarn PnP. It's a little silly in the sense that it defeats the purpose of having ESLint and friends in the Docker image at all. On the other hand, it's generally a good idea to add those directly to your project as dev dependencies anyways to keep your IDE in sync with MegaLinter.

[nvuillam]

@Kurt-von-Laven I'm trying to fix that in #2601 ... the idea is the following:

• check if PRE_COMMANDS contain "npm" or "yarn"
• if yes:
  • copy /node_deps/node_modules into |workspace+"node_modules"
  • Add workspace+"node_modules" in PATH
  • run the PRE_COMMANDS

Do you think it could work ? ^^

[nvuillam]

@Kurt-von-Laven i tried to reproduce your issue and it seems to work... please can you confirm with the newest beta version ? :)

https://github.com/oxsecurity/megalinter/actions/runs/4846602288/jobs/8636240640

[nvuillam]

So, current status:

• the pre-commands are executed in /node-deps ONLY if they contain npm i or yarn add AND if the cwd is "root" (which is by default)
• No more copy of /node-deps anywhere
• Working examples when npm install xxx commands are run with cwd: workspace
• Capability to override XXX_CLI_EXECUTABLE with ["npm", "run", "xxx"] or ["yarn", "run", "xxx"]

[nvuillam]

I make a new pinned issue to ask for tests with beta before the new release

[Kurt-von-Laven]

To reproduce issue 1, I suspect one simply needs a .config/dotnet-tools.json that includes:

{
  "version": 1,
  "isRoot": true,
  "tools": {
    "CSharpier": {
      "version": "0.23.0",
      "commands": ["dotnet-csharpier"]
    }
}

Running dotnet tool restore in the workspace doesn't resolve this issue, but it can be worked around by adding the following to .mega-linter.yaml:

CSHARP_CSHARPIER_PRE_COMMANDS:
  - command: dotnet tool restore
    continue_if_failed: false
    cwd: workspace

I haven't had a chance to investigate what introduced this breaking change, so I don't have a sense of the trade-offs yet. At face value it seems negative.

Issue 2 has been addressed for non-JS projects provided that the npm install command uses cwd: workspace. Avoiding this breaking change is another argument against copying /node-deps/node_modules to /tmp/lint/node_modules.

[mbelangergit]

I've just faced issue 1 with CSharpier, upgrading MegaLinter from 6.22.2 to 7.0.4.

The workaround described above worked (adding dotnet tool restore in CSHARP_CSHARPIER_PRE_COMMANDS) and my runs are now back to green.

But is there a plan to revisit the issue and find a fix inside MegaLinter to avoid users adding the CSHARP_CSHARPIER_PRE_COMMANDS workaround?

[nvuillam]

mmmm @bdovaz , as the dotnet expert, maybe you have an opinion about @mbelangergit issue ? :)

Maybe we should add dotnet tool restore as a default internal pre-command ?

[Kurt-von-Laven]

I was able to reproduce this issue as well. Maybe we could just run dotnet tool restore when building the image if we don't already, but I defer to @bdovaz.