Skip to content
New issue

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

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

Already on GitHub? # to your account

Consistent Jelly version for commons-jelly-tags-fmt #10128

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

basil
Copy link
Member

@basil basil commented Jan 7, 2025

We currently ship commons-jelly-tags-fmt 1.0 from 19 years ago. While the fact that it has not needed any changes in 19 years is a testament to the quality of that code, we were not so lucky in jenkinsci/jelly#110 and jenkinsci/json-lib@b1fe8a3, both of which required us to patch and deliver changes to unmaintained third-party libraries via our forks. We were able to do both easily because we had a pipeline for delivering changes, which we do not currently have for commons-jelly-tags-fmt because the project is dead upstream.

This PR solves this problem by switching our commons-jelly-tags-fmt JAR to the one built and deployed from our fork of Jelly. I compared the sources in our fork to that of https://repo1.maven.org/maven2/commons-jelly/commons-jelly-tags-fmt/1.0/commons-jelly-tags-fmt-1.0-sources.jar and, other than Javadoc updates for Java 11/17/21 compatibility, the sources were identical, so there is no risk here in terms of code change. The only difference in the JARs is that ours is now compiled with Java 17. More importantly, though, this allows us to deliver changes to this code in the future, should the need arise. Currently I do not anticipate such a need.

A similar problem exists for commons-jelly-tags-xml, which I am explicitly not tackling here. I could not easily find the source files for what we are shipping today in https://repo1.maven.org/maven2/commons-jelly/commons-jelly-tags-xml/1.1/ and it is not immediately obvious to me whether or not we are even using this functionality. Some more effort would be required (a) to confirm that we are using this functionality and (b) to dig up the source code for what we are shipping today (or decompile the JAR) to confirm that switching to the sources in our fork would not be introducing a regression.

Testing done

As mentioned above, confirmed that the only source code changes were changes to Javadoc comments, which carry a minimal to zero risk of regression. Automated testing should take care of the rest.

Jelly versions after this change:

WEB-INF/lib/commons-jelly-1.1-jenkins-20250107.jar
WEB-INF/lib/commons-jelly-tags-define-1.1-jenkins-20250107.jar
WEB-INF/lib/commons-jelly-tags-fmt-1.1-jenkins-20250107.jar
WEB-INF/lib/commons-jelly-tags-xml-1.1.jar (see note above)

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

@basil basil added the skip-changelog Should not be shown in the changelog label Jan 7, 2025
Copy link
Member Author

@basil basil left a comment

Choose a reason for hiding this comment

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

Merging this so that we can avoid unnecessary Renovate builds for these two dependencies and so that I can rebase other Stapler PRs on top of these changes.

@basil basil merged commit 84ac21f into jenkinsci:master Jan 7, 2025
15 checks passed
@basil basil deleted the jelly-fmt branch January 7, 2025 19:17
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
skip-changelog Should not be shown in the changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants