Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Segmentation fault (core dumped) #6898

Closed
rcerljenko opened this issue Jun 5, 2023 · 37 comments
Closed

Segmentation fault (core dumped) #6898

rcerljenko opened this issue Jun 5, 2023 · 37 comments
Labels
help wanted is likely non-trival and help is wanted priority: high is impactful on users status: needs investigation triage needs further investigation

Comments

@rcerljenko
Copy link

What steps are needed to reproduce the bug?

Just install latest version.

What Stylelint configuration is needed to reproduce the bug?

NA

How did you run Stylelint?

stylelint --cache <path>

Which version of Stylelint or dependencies are you using?

15.6.3

What did you expect to happen?

Run without errors.

What actually happened?

It crashes with this exception: Segmentation fault (core dumped)

Does the bug relate to non-standard syntax?

No

Proposal to fix the bug

No response

@younky-yang
Copy link

same issue happens when I updated to the latest version of 15.6.3.

@ybiquitous
Copy link
Member

@rcerljenko Thanks for the report. I doubt this is a problem of Cosmiconfig:

Can you confirm a workaround to downgrade Cosmiconfig to 8.1.3?

@ybiquitous ybiquitous added status: needs investigation triage needs further investigation help wanted is likely non-trival and help is wanted labels Jun 5, 2023
@jlherren
Copy link

jlherren commented Jun 5, 2023

Downgrading to cosmiconfig 8.1.3 did solve the issue for me.

@ghost
Copy link

ghost commented Jun 5, 2023

最新バージョン 15.6.3 に更新したときにも同じ問題が発生します。

Me too.

@romainmenke
Copy link
Member

romainmenke commented Jun 5, 2023

I added this in my package.json to temporarily avoid the issues :

(I use npm)

  "overrides": {
    "stylelint": {
      "cosmiconfig": "8.1.3"
    }
  },

@tjx666
Copy link

tjx666 commented Jun 5, 2023

I also occur this problem:

lQLPJx-0LdicJQLMrM0E1LCs79agXXVZTAR0XIPlwBIA_1236_172

"stylelint": "^15.6.3"

This issue can occasionally be reproduced on my MBP, but it can be reproduced every time on the CI Linux.

image

@tjx666
Copy link

tjx666 commented Jun 5, 2023

I can confirmed lock "cosmiconfig": "8.1.3" works

@lucgagan
Copy link

lucgagan commented Jun 5, 2023

Confirmed that:

  "pnpm": {
    "overrides": {
      "cosmiconfig": "8.1.3"
    }
  },

fixes the issue

@d-fischer
Copy link

d-fischer commented Jun 5, 2023

Hi, cosmiconfig maintainer here.

For some reason, within stylelint, any call to import() crashes. Do you have any idea why that could be? In general, even though stylelint is written in CJS. it shouldn't have any problems calling dynamic import()...

I have tried running the full prettier test suite, no issues with that... I'm not sure what's different here.

@romainmenke
Copy link
Member

romainmenke commented Jun 5, 2023

This seems relevant : https://github.com/stylelint/stylelint/blob/main/bin/stylelint.js#L6

If I remove that, the issue goes away.


This seems to match some of the keywords above : zertosh/v8-compile-cache#30

@alexander-akait
Copy link
Member

Oh, yes v8-compile-cache and dynamic imports broken

@alexander-akait
Copy link
Member

It is an old problem zertosh/v8-compile-cache#30

@alexander-akait
Copy link
Member

alexander-akait commented Jun 5, 2023

There is no good solution, because vm for ESM is still experimantal and still have problems with dynamic imports and memory leaking, so there is only one solution, remove v8-compile-cache usage

@romainmenke
Copy link
Member

This still fails.

Is it possible that jest also isn't compatible?

@alexander-akait
Copy link
Member

Yeah, there is nodejs/node#35889 and nodejs/node#35889 (comment), ESM is still not stable as we want 😞

@d-fischer
Copy link

Well, uh, that makes sense. cosmiconfig migrated to vitest for this reason. Apparently, the switch is relatively straightforward.

@romainmenke
Copy link
Member

romainmenke commented Jun 5, 2023

Given that the issue doesn't seem trivial to resolve I think it might be best to first help those users who are affected by this.

@d-fischer Would you consider doing a rollback of the changes in 8.2 as they seem to be breaking? A semver major would be less disruptive for a feature addition like this, as users of Stylelint wouldn't get that next major unless a change is also made here first.

If that is not possible I think we should release a version of Stylelint with cosmiconfig pinned to 8.1.3. This will takes the pressure of and ensure that there is only a limited proliferation of "overrides": { "stylelint": { "cosmiconfig": "8.1.3" } }.

When the issues are resolved we can unpin the version and users won't have to take manual actual.

@alexander-akait @ybiquitous thoughts?

@alexander-akait
Copy link
Member

alexander-akait commented Jun 5, 2023

@d-fischer I'll tell you a secret - vitest has the same problem, because leaking and broken dynamic import cache in v8, not in node.js or jest

@alexander-akait
Copy link
Member

@romainmenke

If that is not possible I think we should release a version of Stylelint with cosmiconfig pinned to 8.1.3. This will takes the pressure of and ensure that there is only a limited proliferation of "overrides": { "stylelint": { "cosmiconfig": "8.1.3" } }.

yeah, it is the best solution right now - pin it and open an issue (or rename this) and do a patch release

@d-fischer
Copy link

d-fischer commented Jun 5, 2023

@romainmenke

yeah, it is the best solution right now - pin it and open an issue (or rename this) and do a patch release

The removal of v8-compiler-cache would still be required, I think. I'm updating #6901 with the pin.

Nvm, I misunderstood.

@d-fischer
Copy link

@d-fischer I'll tell you a secret - vitest has the same problem, because leaking and broken dynamic import cache in v8, not in node.js or jest

Switching from jest to vitest fixed the problems of cosmiconfig's test suite with dynamic imports, that's all I can say.

@ybiquitous ybiquitous mentioned this issue Jun 5, 2023
4 tasks
@d-fischer
Copy link

d-fischer commented Jun 5, 2023

My concern with that is that it might be passing the issue to users of Stylelint.

So how likely do you think it is that people will invoke stylelint as part of their test suite?

The new ESM loader of cosmiconfig is pretty much exactly what prettier has been shipping for a while, plus an additional try-catch (which doesn't catch this error because it's a segfault, sadly). I have not tested it yet, but it should be pretty much the exact same situation, but prettier didn't ever get a report about that.

To be honest, I don't have any clue how to fix this problem without just getting rid of ESM loading again. This is an ecosystem issue.

@d-fischer
Copy link

@d-fischer Is there any reason not to use the sync loaders?

Should be fine; as cosmiconfig uses await, a non-promise result should be handled just fine. The only (obscure) breakage I could imagine would be a file that includes a then property at the root level, which might just crash with an obscure error. If you want to be extra sure about that corner case, I guess you could do something like

'.js': (path, content) => Promise.resolve(defaultLoadersSync['.js'](path, content)),

@ybiquitous
Copy link
Member

ybiquitous commented Jun 5, 2023

Thanks for the quick fix #6902.

To confirm, if people try using .stylelintrc.mjs (ESM), can #6902 prevent this problem?

@d-fischer
Copy link

To confirm, if people try using .stylelintrc.mjs (ESM), can #6902 prevent this problem?

I guess you would still have to remove that weird cache package for that to work.

@romainmenke
Copy link
Member

To confirm, if people try using .stylelintrc.mjs (ESM), can #6902 prevent this problem?

No, or at least not intentionally.

I am unsure what the behavior was before when people tried to use .stylelintrc.mjs.

Is this something that was supported and should be tested?
Is this something that wasn't supported and should be handled by a nice error message?

@ybiquitous
Copy link
Member

Currently, I don't think we should support .mjs explicitly. See also below (CLI help and doc):

stylelint/lib/cli.js

Lines 115 to 118 in f1ace61

- a "stylelint" property in "package.json"
- a ".stylelintrc" file (with or without filename extension:
".json", ".yaml", ".yml", ".js", and ".cjs" are available)
- a "stylelint.config.js" file exporting a JS object

- `stylelint` property in `package.json`
- `.stylelintrc` file
- `.stylelintrc.{cjs,js,json,yaml,yml}` file
- `stylelint.config.{cjs,js}` file exporting a JS object

But, we note that some people may try using .mjs if Cosmiconfig 8.2 is installed. Let's consider it later, though.

@romainmenke
Copy link
Member

Yeah, should just work if we remove v8-compiler-cache as @d-fischer said and could be tested with a simpler separate script that doesn't rely on jest :)

@alexander-akait
Copy link
Member

alexander-akait commented Jun 5, 2023

Just infromation - we use this https://github.com/webpack-contrib/postcss-loader/blob/master/src/utils.js#L37 for js/mjs/cjs and everything works for a lot of developers

@alexander-akait
Copy link
Member

Shorty - many tools and bundlers handle import(...) and most of the time it's expected. BUT sometimes you don't want to do it, that is why you can use non analizable code like new Function(...)

@ybiquitous
Copy link
Member

I agree with removing v8-compiler-cache if there are no more concerns. 👍🏼

@ybiquitous
Copy link
Member

Great thanks, @romainmenke! I've merged #6902.

I'll do a patch release today. See also #6903.

@ybiquitous
Copy link
Member

https://github.com/stylelint/stylelint/releases/tag/15.7.0 is released! 🎉
The segmentation fault should not occur. 👍🏼

@ybiquitous
Copy link
Member

I've opened follow-up issues:

So, I'm closing this issue since it's no longer needed. Feel free to comment if you still have any concerns.

Thank you so much for helping us resolve this problem!

rejas pushed a commit to MagicMirrorOrg/MagicMirror that referenced this issue Jun 6, 2023
…3118)

- update electron to v25
- added `overrides:` section to `package.json` to fix
stylelint/stylelint#6898 (must be removed when
fixed upstream, fixing the version to `v15.6.2` did not fix the problem)
- update other deps
beatrizsmerino added a commit to beatrizsmerino/nuxt-course that referenced this issue Jun 6, 2023
mergify bot pushed a commit to kc-workspace/kcws-js that referenced this issue Jun 6, 2023
* chore: migrate

* feat: migrate pnpmOptions to pnpm-config file

* feat: enabled some extra experiment options

* chore: update repo-state

* fix(rush-api-documenter): v1 didn't generate _docs on correct location

* chore: reorder vscode directory alphabetical

* perf(deps): upgrade `typedoc-plugin-missing-exports` to v2 to support latest typedoc

* feat: re-work typedoc configuration for new version 0.24

* perf(deps): upgrade all minor and patch dependencies

* perf(rush-dependencies-updater): upgrade latest by default

* feat: add patch for eslint module resolver issue

* perf(deps): upgrade major dependencies

* feat(web-rig): upgrade heft to v0.51.0

* chore: add debug profile for debugging rush build

* perf(node-rig): upgrade heft to v0.51.0

* chore(deps): update lock files and approved packages

* perf(web-rig): sync configs from node-rig-package

* perf(reset.css): upgrade stylelint config to new version

* feat: migrate all package to new heft v0.51.0

* chore: update pnpm lock file

* perf(rigs): add heft-lint-plugin patch fixes microsoft/rushstack#4160

* chore(rush): update changefiles

* perf: remove patches as it not works

* perf: remove cyclic dependencies

* chore: update lock file

* chore: sync tsconfig from node-rig

* perf(rush): `_phase:test` now optional, and remove `_phase:test-types`

* chore: remove `_phase:test-types`

* perf(rush): add _phase:test output names

* fix(rigs): update rush-project config for caching test result

* perf(deps): update lock file

* perf(reset.css): sass no longer required and fixed type when publish

* chore: remove unused file

* perf(web-rig): remove webpack5 because license issues

* chore: export scss typing at temp/scss-ts/ instead

* perf(css): export typings to lib/types instead of temp directory

* perf: add heft-lint-plugin patch to fix create() error

* chore: remove old patch

* fix(reset.css): cannot build on ci machine

* chore: override gitleaks confg

* chore(temp-fix): force add patch file for heft-lint-plugin package

* fix: downgrade cosmiconfig as commented by stylelint/stylelint#6898

* fix(lint): incorrect usage of the term: “Css”, use “CSS” instead

* perf(deps): upgrade @typescript-eslint/typescript-estree

* chore: upgrade typedoc patch version

* fix(stylelint): update stylelint to fixed segmentation fault

* fix(heft): upgrade heft-plugins to fix eslint failed

* chore(deps): update typescript eslint

* chore: remove patch file

* feat(git-hooks): add commit message lint and pre-push check

* chore: update change files

* perf: bring test-types back as on CI might logs warning even it successfully completed

* perf: separate testing types to different script for enabled parallel

* feat(rush-commitlint): add commitizen to generate git commit with conventional syntax

* docs(core): add new everyday task for create git commit

* fix(types): add missing build and clean phase as required by rush commandline
msljivic added a commit to enigmatry/entry-angular-building-blocks that referenced this issue Jun 15, 2023
msljivic added a commit to enigmatry/entry-angular-building-blocks that referenced this issue Jun 15, 2023
* Fix stylelint issue 'Segmentation fault'. Bump stylelint to v15.7.0.
stylelint/stylelint#6898

* Bump @enigmatry/stylelint-config to latest.

* Delete deploy-stylelint-config.yml
ybiquitous added a commit that referenced this issue Nov 13, 2023
This workaround was introduced in #6902. See also #6898.
backspace added a commit to backspace/adventures that referenced this issue Dec 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted is likely non-trival and help is wanted priority: high is impactful on users status: needs investigation triage needs further investigation
Development

Successfully merging a pull request may close this issue.

9 participants