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 KXML2 library #8503

Merged
merged 2 commits into from
Sep 27, 2023
Merged

Remove KXML2 library #8503

merged 2 commits into from
Sep 27, 2023

Conversation

basil
Copy link
Member

@basil basil commented Sep 18, 2023

This PR removes the following JAR from the WAR:

  • kxml2-2.3.0.jar

Core stopped depending on it in #7778. In that PR, I wrote:

I am not yet removing the kxml2 JAR file from the Jenkins WAR in this PR because I have not done the research to determine whether doing so would affect any plugins. If, once this change has been tested in several weekly releases and proven to be stable, there is a desire to remove this no-longer-needed JAR file, the removal can be explored at that time.

The above change has been shipping for a while now and we have not had a need to return to using KXML2 in core, so I am proceeding with the removal of this library from core. As you will see below, nothing of any relevance in the ecosystem is still using this functionality, and any stragglers are better off carrying this component themselves rather than relying on core for it.

I scanned the update centers for Jenkins and CloudBees and while I found some stuff using XPP3, I found nothing directly using KXML2.

Testing done

The PR build and update center scan should be sufficient. I also did a BOM run with this PR with no issues.

Proposed changelog entries

  • Stop shipping net.sf.kxml:kxml2 because Jenkins no longer depends on it.

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

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

Maintainer checklist

@basil basil added on-hold This pull request depends on another event/release, and it cannot be merged right now upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed removed This PR removes a feature or a public API labels Sep 18, 2023
basil added a commit to basil/testng-plugin-plugin that referenced this pull request Sep 18, 2023
basil added a commit to basil/vectorcast-coverage-plugin that referenced this pull request Sep 18, 2023
basil added a commit to basil/vectorcast-coverage-plugin that referenced this pull request Sep 18, 2023
MarkEWaite pushed a commit to jenkinsci/testng-plugin-plugin that referenced this pull request Sep 18, 2023
@basil
Copy link
Member Author

basil commented Sep 18, 2023

This change seems to break new XStream(), which uses the default driver (still XML Pull Parser v3), but not new XStream2(), which uses the StaX driver. In most cases we're already using (or should be using) new XStream2(), but there are still a handful of cases (mostly in tests) where we still need to use new XStream(). To get those uses to work with the removal of all three JARs mentioned above, they need to be changed to new XStream(XStream2.getDefaultDriver()). I'll explore how much work is needed to adapt the ecosystem to this change, and either do that work or reduce this scope of this PR by retaining at least one of the three JARs.

@basil basil added the work-in-progress The PR is under active development, not ready to the final review label Sep 18, 2023
@basil basil marked this pull request as draft September 18, 2023 23:23
basil added a commit to basil/bom that referenced this pull request Sep 21, 2023
@basil
Copy link
Member Author

basil commented Sep 21, 2023

While attempting to remove all three libraries (kxml2-2.3.0.jar, mxparser-1.2.2.jar, and xpp3-1.1.4c.jar) did lead me to file a few clean up PRs in various repositories to make plugin code more consistent with core (which was a nice benefit), I ended up giving up on removing mxparser after finding that it broke new XStream(), which would break third-party library code that happens to use XStream (independent of Jenkins plugins or Jenkins core itself) and I ended up giving up on removing xpp3 after finding some proprietary plugins that directly imported XppDriver. So the scope of this PR is now just removing kxml2 which should be completely safe to do today. Therefore I am removing the on-hold label.

@basil basil changed the title Remove old XML Pull Parser v3 libraries Remove KXML2 library Sep 21, 2023
@basil basil added skip-changelog Should not be shown in the changelog and removed work-in-progress The PR is under active development, not ready to the final review on-hold This pull request depends on another event/release, and it cannot be merged right now upgrade-guide-needed This changes might be breaking in rare circumstances, an entry in the LTS upgrade guide is needed removed This PR removes a feature or a public API labels Sep 22, 2023
@basil basil marked this pull request as ready for review September 22, 2023 00:01
@amuniz
Copy link
Member

amuniz commented Sep 22, 2023

ended up giving up on removing xpp3 after finding some proprietary plugins that directly imported XppDriver

I think its usage in that particular proprietary plugin (operations-center-context) can be just removed (as long as there is an XStream constructor not requiring an instance of it).

@basil
Copy link
Member Author

basil commented Sep 22, 2023

I think its usage in that particular proprietary plugin […] can be just removed

I am sure that it can be. But the fact that it has not been is the reason that I have given up on this removal and reduced the scope of this PR.

basil added a commit to basil/bom that referenced this pull request Sep 23, 2023
@basil basil added the pct-successful This PR has successfully passed the full plugin-compatibility-test suite label Sep 23, 2023
@NotMyFault NotMyFault requested a review from a team September 24, 2023 09:23
@NotMyFault
Copy link
Member

/label ready-for-merge


This PR is now ready for merge. We will merge it after ~24 hours if there is no negative feedback.
Please see the merge process documentation for more information about the merge process.
Thanks!

@comment-ops-bot comment-ops-bot bot added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Sep 26, 2023
@NotMyFault NotMyFault added removed This PR removes a feature or a public API and removed skip-changelog Should not be shown in the changelog labels Sep 27, 2023
@NotMyFault NotMyFault merged commit 9dabd87 into jenkinsci:master Sep 27, 2023
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pct-successful This PR has successfully passed the full plugin-compatibility-test suite ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback removed This PR removes a feature or a public API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants