Closed nvuillam closed 1 year ago
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
It seems for the moment the only major error is npm package dependencies ... maybe the issue with csharpier is just a build issue :/
If I understand correctly, the CSharpier issue means MegaLinter will break for anyone with C# files who didn't disable CSharpier.
I think the third one may be related to typescript-eslint/typescript-eslint#1592. It's also similar to typescript-eslint/typescript-eslint#5223.
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/node18-strictest-esm@1.0.1
- 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.
So... maybe we should not force the cd node_deps when we have a npm install instruction ?
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/node18-strictest-esm@1.0.1
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.
@Kurt-von-Laven I'm trying to fix that in https://github.com/oxsecurity/megalinter/pull/2601 ... the idea is the following:
/node_deps/node_modules
into |workspace+"node_modules"
Do you think it could work ? ^^
@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
So, current status:
npm i
or yarn add
AND if the cwd is "root" (which is by default)cwd: workspace
["npm", "run", "xxx"]
or ["yarn", "run", "xxx"]
I make a new pinned issue to ask for tests with beta before the new release
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
.
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?
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 ?
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.
CSharpier is broken; I am curious whether anyone else encounters this issue:
CSpell is broken when importing dictionaries that aren't bundled:
Our
cspell.config.yaml
contains:Our
.mega-linter.yaml
contains:In our Yarn TypeScript projects, there is one error per file of the following form:
Our
tsconfig.json
contains:Our
package.json
contains:Our
.eslintrc.yaml
is:Originally posted by @Kurt-von-Laven in https://github.com/oxsecurity/megalinter/issues/1877#issuecomment-1516142425