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

Remove JTidy from core #5521

Merged
merged 1 commit into from May 25, 2021
Merged

Remove JTidy from core #5521

merged 1 commit into from May 25, 2021

Conversation

basil
Copy link
Member

@basil basil commented May 24, 2021

Jenkins core ships JTidy 4aug2000r7-dev-hudson-1, a fork of JTidy 04aug2000r7-dev. The original 04aug2000r7-dev release was published on August 1, 2001; the fork was published on July 14, 2008. JTidy was forked by Kohsuke in commit 2fcca36 as part of the fix for JENKINS-1680 to resolve an incompatibility with IBM WebSphere 6.1, which itself reached end-of-life on September 30, 2013.

Carrying this ancient fork of an even more ancient dependency does not seem desirable for the Jenkins project. In recent years there have been efforts to revive the JTidy project on GitHub, but rather than migrating to this revival project I think it makes more sense to remove the JTidy dependency from core. The dependency is effectively unused throughout the entire Jenkins ecosystem, so there is no strong reason to keep bundling it.

Nothing in Jenkins core itself uses JTidy. Running the classes from the JTidy JAR through usage-in-plugins and searching for usages in sources, I found only two references in plugins:

  • JDepend, a plugin with 2,313 installations last released 3 years ago. Although this plugin imported a JTidy class and had a method that used that class, the only caller of that method was commented out. I opened Remove usage of Tidy jdepend-plugin#5 to remove this dead code. Although this plugin is not actively maintained, Oleg has permissions and may be able to help with merging and releasing that PR.
  • NIS notification lamp, a plugin with 5 installations last released 8 years ago. This plugin does actually use JTidy, so it would be better for it to directly depend on JTidy rather than relying on Jenkins core to provide this dependency. I opened Directly depend on JTidy rather than relying on Jenkins core nis-notification-lamp-plugin#5 to explicitly specify this dependency. This plugin is not actively maintained, so the likelihood of a merge and release of that PR are quite low. But with only 5 installations, I do not think we should worry about this.

Proposed changelog entries

Developer: Remove JTidy dependency from Jenkins core. Plugins that use JTidy functionality, including NIS notification lamp, must be updated to explicitly declare a dependency on JTidy rather than relying on Jenkins core to provide this library.

Proposed upgrade guidelines

The JTidy dependency has been removed from Jenkins core. Users of JDepend must upgrade JDepend to the latest version. Other plugins that use JTidy functionality, including NIS notification lamp, must be updated to explicitly declare a dependency on JTidy rather than relying on Jenkins core to provide this library.

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 correct
  • 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 dependencies Pull requests that update a dependency file developer Changes which impact plugin developers upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed labels May 24, 2021
@daniel-beck
Copy link
Member

I have the same results with usage-in-plugins, only the two plugins use it and only the single class. The PR build doesn't have the dependency bundled anymore, otherwise no differences.

usage-in-plugins classes file I used
org/w3c/tidy/AttrCheck
org/w3c/tidy/AttrCheckImpl
org/w3c/tidy/AttrCheckImpl$CheckAlign
org/w3c/tidy/AttrCheckImpl.CheckAlign
org/w3c/tidy/AttrCheckImpl$CheckBool
org/w3c/tidy/AttrCheckImpl.CheckBool
org/w3c/tidy/AttrCheckImpl$CheckId
org/w3c/tidy/AttrCheckImpl.CheckId
org/w3c/tidy/AttrCheckImpl$CheckName
org/w3c/tidy/AttrCheckImpl.CheckName
org/w3c/tidy/AttrCheckImpl$CheckScript
org/w3c/tidy/AttrCheckImpl.CheckScript
org/w3c/tidy/AttrCheckImpl$CheckUrl
org/w3c/tidy/AttrCheckImpl.CheckUrl
org/w3c/tidy/AttrCheckImpl$CheckValign
org/w3c/tidy/AttrCheckImpl.CheckValign
org/w3c/tidy/Attribute
org/w3c/tidy/AttributeTable
org/w3c/tidy/AttVal
org/w3c/tidy/CheckAttribs
org/w3c/tidy/CheckAttribsImpl
org/w3c/tidy/CheckAttribsImpl$CheckAnchor
org/w3c/tidy/CheckAttribsImpl.CheckAnchor
org/w3c/tidy/CheckAttribsImpl$CheckAREA
org/w3c/tidy/CheckAttribsImpl.CheckAREA
org/w3c/tidy/CheckAttribsImpl$CheckCaption
org/w3c/tidy/CheckAttribsImpl.CheckCaption
org/w3c/tidy/CheckAttribsImpl$CheckHR
org/w3c/tidy/CheckAttribsImpl.CheckHR
org/w3c/tidy/CheckAttribsImpl$CheckHTML
org/w3c/tidy/CheckAttribsImpl.CheckHTML
org/w3c/tidy/CheckAttribsImpl$CheckIMG
org/w3c/tidy/CheckAttribsImpl.CheckIMG
org/w3c/tidy/CheckAttribsImpl$CheckLINK
org/w3c/tidy/CheckAttribsImpl.CheckLINK
org/w3c/tidy/CheckAttribsImpl$CheckMap
org/w3c/tidy/CheckAttribsImpl.CheckMap
org/w3c/tidy/CheckAttribsImpl$CheckSCRIPT
org/w3c/tidy/CheckAttribsImpl.CheckSCRIPT
org/w3c/tidy/CheckAttribsImpl$CheckSTYLE
org/w3c/tidy/CheckAttribsImpl.CheckSTYLE
org/w3c/tidy/CheckAttribsImpl$CheckTABLE
org/w3c/tidy/CheckAttribsImpl.CheckTABLE
org/w3c/tidy/CheckAttribsImpl$CheckTableCell
org/w3c/tidy/CheckAttribsImpl.CheckTableCell
org/w3c/tidy/Clean
org/w3c/tidy/Configuration
org/w3c/tidy/Dict
org/w3c/tidy/DOMAttrImpl
org/w3c/tidy/DOMAttrMapImpl
org/w3c/tidy/DOMCDATASectionImpl
org/w3c/tidy/DOMCharacterDataImpl
org/w3c/tidy/DOMCommentImpl
org/w3c/tidy/DOMDocumentImpl
org/w3c/tidy/DOMDocumentTypeImpl
org/w3c/tidy/DOMElementImpl
org/w3c/tidy/DOMExceptionImpl
org/w3c/tidy/DOMNodeImpl
org/w3c/tidy/DOMNodeListByTagNameImpl
org/w3c/tidy/DOMNodeListImpl
org/w3c/tidy/DOMProcessingInstructionImpl
org/w3c/tidy/DOMTextImpl
org/w3c/tidy/Entity
org/w3c/tidy/EntityTable
org/w3c/tidy/IStack
org/w3c/tidy/Lexer
org/w3c/tidy/Lexer$W3CVersionInfo
org/w3c/tidy/Lexer.W3CVersionInfo
org/w3c/tidy/MutableBoolean
org/w3c/tidy/MutableInteger
org/w3c/tidy/MutableObject
org/w3c/tidy/Node
org/w3c/tidy/Out
org/w3c/tidy/OutImpl
org/w3c/tidy/Parser
org/w3c/tidy/ParserImpl
org/w3c/tidy/ParserImpl$ParseBlock
org/w3c/tidy/ParserImpl.ParseBlock
org/w3c/tidy/ParserImpl$ParseBody
org/w3c/tidy/ParserImpl.ParseBody
org/w3c/tidy/ParserImpl$ParseColGroup
org/w3c/tidy/ParserImpl.ParseColGroup
org/w3c/tidy/ParserImpl$ParseDefList
org/w3c/tidy/ParserImpl.ParseDefList
org/w3c/tidy/ParserImpl$ParseFrameSet
org/w3c/tidy/ParserImpl.ParseFrameSet
org/w3c/tidy/ParserImpl$ParseHead
org/w3c/tidy/ParserImpl.ParseHead
org/w3c/tidy/ParserImpl$ParseHTML
org/w3c/tidy/ParserImpl.ParseHTML
org/w3c/tidy/ParserImpl$ParseInline
org/w3c/tidy/ParserImpl.ParseInline
org/w3c/tidy/ParserImpl$ParseList
org/w3c/tidy/ParserImpl.ParseList
org/w3c/tidy/ParserImpl$ParseNoFrames
org/w3c/tidy/ParserImpl.ParseNoFrames
org/w3c/tidy/ParserImpl$ParseOptGroup
org/w3c/tidy/ParserImpl.ParseOptGroup
org/w3c/tidy/ParserImpl$ParsePre
org/w3c/tidy/ParserImpl.ParsePre
org/w3c/tidy/ParserImpl$ParseRow
org/w3c/tidy/ParserImpl.ParseRow
org/w3c/tidy/ParserImpl$ParseRowGroup
org/w3c/tidy/ParserImpl.ParseRowGroup
org/w3c/tidy/ParserImpl$ParseScript
org/w3c/tidy/ParserImpl.ParseScript
org/w3c/tidy/ParserImpl$ParseSelect
org/w3c/tidy/ParserImpl.ParseSelect
org/w3c/tidy/ParserImpl$ParseTableTag
org/w3c/tidy/ParserImpl.ParseTableTag
org/w3c/tidy/ParserImpl$ParseText
org/w3c/tidy/ParserImpl.ParseText
org/w3c/tidy/ParserImpl$ParseTitle
org/w3c/tidy/ParserImpl.ParseTitle
org/w3c/tidy/PPrint
org/w3c/tidy/Report
org/w3c/tidy/StreamIn
org/w3c/tidy/StreamInImpl
org/w3c/tidy/Style
org/w3c/tidy/StyleProp
org/w3c/tidy/TagTable
org/w3c/tidy/Tidy
org/w3c/tidy/TidyBeanInfo

(Including both variants of class name is easier than remembering which one is used)

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.

Looks like a reasonable small cleanup with very little negative impact 👍

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

No objections. This dependency is too old to keep maintaining it in the core. JDepends plugin was updated for JEP-200 and Co, and other plugins can be trivially updated as well

@oleg-nenashev oleg-nenashev requested a review from a team May 24, 2021 13:40
@oleg-nenashev
Copy link
Member

We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file developer Changes which impact plugin developers ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed
Projects
None yet
6 participants