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

Revert #6749 #6774

Closed
wants to merge 1 commit into from
Closed

Revert #6749 #6774

wants to merge 1 commit into from

Conversation

basil
Copy link
Member

@basil basil commented Jul 4, 2022

#6749 caused the following issue when running mvn clean verify:

[INFO] $ stylelint src/main/less --syntax less
[INFO] Error: The "syntax" option is no longer available. You should install an appropriate syntax, e.g. postcss-scss, and use the "customSyntax" option
[INFO]     at getPostcssResult (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/getPostcssResult.js:37:25)
[INFO]     at lintSource (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/lintSource.js:79:20)
[INFO]     at async /home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/standalone.js:228:27
[INFO]     at async Promise.all (index 2)
[INFO]     at async standalone (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/standalone.js:267:22)

The solution is described in stylelint/stylelint#5663 (comment) and the migration guide. Prototyping that solution by adding customSyntax: "postcss-less" to .stylelintrc.js and removing --syntax less from package.json fixes the first problem but reveals a second problem:

[INFO] $ stylelint src/main/less
[INFO] TypeError: this.getPosition is not a function
[INFO]     at LessParser.inlineComment (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/LessParser.js:71:28)
[INFO]     at LessParser.isInlineComment (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/nodes/inline-comment.js:43:12)
[INFO]     at LessParser.other (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/LessParser.js:156:36)
[INFO]     at LessParser.parse (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss/lib/parser.js:75:16)
[INFO]     at parse (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/index.js:13:12)
[INFO]     at new LazyResult (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/node_modules/postcss/lib/lazy-result.js:133:16)
[INFO]     at getPostcssResult (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/getPostcssResult.js:77:30)
[INFO]     at async lintSource (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/lintSource.js:79:4)
[INFO]     at async /home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/standalone.js:228:27
[INFO]     at async Promise.all (index 0)

Again prototyping the solutions described in stylelint/stylelint#5663 (comment) and stylelint/stylelint#5663 (comment) fixes the second problem but reveals a third problem:

[INFO] $ stylelint src/main/less
[INFO] TypeError: this.getPosition is not a function
[INFO]     at LessParser.inlineComment (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/LessParser.js:71:28)
[INFO]     at LessParser.isInlineComment (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/nodes/inline-comment.js:43:12)
[INFO]     at LessParser.other (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/LessParser.js:156:36)
[INFO]     at LessParser.parse (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss/lib/parser.js:75:16)
[INFO]     at parse (/home/basil/src/jenkinsci/jenkins/war/node_modules/postcss-less/lib/index.js:13:12)
[INFO]     at new LazyResult (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/node_modules/postcss/lib/lazy-result.js:133:16)
[INFO]     at getPostcssResult (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/getPostcssResult.js:77:30)
[INFO]     at async lintSource (/home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/lintSource.js:79:4)
[INFO]     at async /home/basil/src/jenkinsci/jenkins/war/node_modules/stylelint/lib/standalone.js:228:27
[INFO]     at async Promise.all (index 0)

Based on stylelint/stylelint#5713 this seems to reveal that we are using an outdated set of stylelint rules that need to be upgraded in lockstep with stylelint itself.

Clearly #6749 lacked sufficient testing. I am reverting it.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

  • (If applicable) Jira issue is well described
  • Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change) and are in the imperative mood. Examples
    • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
  • Appropriate autotests or explanation to why this change has no tests
  • New public classes, fields, and methods are annotated with @Restricted or have @since TODO Javadoc, as appropriate.
  • New deprecations are annotated with @Deprecated(since = "TODO") or @Deprecated(forRemoval = true, since = "TODO") if applicable.
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

  • There are at least 2 approvals for the pull request and no outstanding requests for change
  • Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
  • Changelog entries in the PR title and/or Proposed changelog entries are accurate, human-readable, and in the imperative mood
  • Proper changelog labels are set so that the changelog can be generated automatically
  • If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
  • If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

@basil basil added the regression-fix Pull request that fixes a regression in one of the previous Jenkins releases label Jul 4, 2022
@NotMyFault
Copy link
Member

caused the following issue when running mvn clean verify:

Interesting, checking out the newest master revision, I don't receive these errors building with named command 🤔

@basil
Copy link
Member Author

basil commented Jul 4, 2022

I don't know what to tell you, Alex.

The regression is visible in the CI build:

[2022-07-04T17:44:09.801Z] [INFO] $ stylelint src/main/less --syntax less
[2022-07-04T17:44:10.123Z] [INFO] Error: The "syntax" option is no longer available. You should install an appropriate syntax, e.g. postcss-scss, and use the "customSyntax" option
[2022-07-04T17:44:10.123Z] [INFO]     at getPostcssResult (/home/jenkins/workspace/Core_jenkins_master/war/node_modules/stylelint/lib/getPostcssResult.js:37:25)
[2022-07-04T17:44:10.123Z] [INFO]     at lintSource (/home/jenkins/workspace/Core_jenkins_master/war/node_modules/stylelint/lib/lintSource.js:79:20)
[2022-07-04T17:44:10.123Z] [INFO]     at async /home/jenkins/workspace/Core_jenkins_master/war/node_modules/stylelint/lib/standalone.js:228:27
[2022-07-04T17:44:10.123Z] [INFO]     at async Promise.all (index 0)
[2022-07-04T17:44:10.123Z] [INFO]     at async standalone (/home/jenkins/workspace/Core_jenkins_master/war/node_modules/stylelint/lib/standalone.js:267:22)

@NotMyFault
Copy link
Member

Yeah I see, kinda unfortunate that we shift all linting errors to warnings at the moment, otherwise this would have been noticed much earlier.

I'm currently looking into it NotMyFault@f42e0d8
Your findings have been addressed so far, but I'm running into the removal of function-calc-no-invalid, which we don't even use.

@NotMyFault NotMyFault mentioned this pull request Jul 4, 2022
12 tasks
Copy link
Member

@daniel-beck daniel-beck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

88a474d4b8453c8b923c4f0741b107d5f2fa94c8^...basil:stylelint-revert looks reasonable. What's left changed in these files is from 43862df and d16cec9.

@basil
Copy link
Member Author

basil commented Jul 4, 2022

Yeah I see, kinda unfortunate that we shift all linting errors to warnings at the moment, otherwise this would have been noticed much earlier.

All the more reason to address JENKINS-68903.

@NotMyFault
Copy link
Member

Yeah I see, kinda unfortunate that we shift all linting errors to warnings at the moment, otherwise this would have been noticed much earlier.

All the more reason to address JENKINS-68903.

Indeed, I've risen #6776 as alternative approach to your PR to address the exceptions without reverting the original change. According to https://ci.jenkins.io/job/Core/job/jenkins/job/PR-6776/1/consoleFull, exceptions are no longer thrown, just the "warnings".

@basil
Copy link
Member Author

basil commented Jul 5, 2022

Closing in favor of #6776.

@basil basil closed this Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
3 participants