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

[JENKINS-10629] Migrate from Commons Compress to Ant #9311

Merged
merged 1 commit into from
May 27, 2024

Conversation

basil
Copy link
Member

@basil basil commented May 25, 2024

We originally used Ant for tar/untar functionality, but we migrated to Commons Compress in #1670, which also added the Commons Compress library to the WAR. Fast forward to today, and recent releases of Commons Compress depend on Commons Lang 3. We don't want to keep increasing the API surface area of the WAR by pulling in these random libraries. And pulling in Commons Lang 3 would override the library plugin we ship today.

A solution can be found by migrating from Commons Compress back to Ant. Our original reason for migrating away from it was that it did not support large files > 8 GiB in size. Today, it does, and I verified this with an automated test. We are unlikely to ever remove Ant from Jenkins core, as it is used in a lot of places in core, so why not use it for tar/untar functionality as well. And unlike Commons Compress, it has no dependency on Commons Lang 3.

In the meantime, we're still blocked from upgrading to the latest release of Commons Compress, because the latest release depends on Commons Lang 3 which we do not want to introduce. But this PR shows a path forward. With core's dependency on Commons Compress removed, plugins can then be adapted, and we can eventually remove Commons Compress from core. Plugins could be adapted by either migrating to core's copy of Ant (as in this PR) or a new Commons Compress library plugin could be created and plugins migrated to that instead. That Commons Compress library plugin could depend on Commons Lang 3 as a plugin-to-plugin dependency.

Testing done

Ran the new test and it passed after about 2 minutes. I am not enabling it by default as it requires a lot of time and disk space to run.

Proposed changelog entries

N/A

Proposed upgrade guidelines

N/A

Submitter checklist

Preview Give feedback

Desired reviewers

@mention

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

Maintainer checklist

Preview Give feedback

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

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

Change looks very good to me. Thanks for the test and thanks for not running it by default.

Copy link
Member

@NotMyFault NotMyFault left a comment

Choose a reason for hiding this comment

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

Thanks for providing the detailed rationale!

@MarkEWaite
Copy link
Contributor

This PR is now ready for merge. We will merge it after approximately 24 hours if there is no negative feedback.

/label ready-for-merge

@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 May 25, 2024
@MarkEWaite MarkEWaite merged commit c10cafc into jenkinsci:master May 27, 2024
16 checks passed
@basil basil deleted the tar branch May 27, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants