Inline icon-set lib

[timja]

See JENKINS-XXXXX.

Iconography is soon to be revamped, see #5065 for some of the changes coming.

It would be a lot easier if iconset lib is in core for this...

Any objections?

Proposed changelog entries

• Entry 1: iconset library inlined into Jenkins core, developers using this should remove the dependency
• ...

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). 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
• 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 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).

[fqueiruga]

LGTM.

What could be the impact on plugins that import the icon-set library, such as https://github.com/jenkinsci/parameterized-trigger-plugin or jenkinsci/authorize-project-plugin

Search link: https://github.com/search?l=Maven+POM&q=org%3Ajenkinsci+icon-set&type=Code

To clarify, what happens with plugins that import icon-set and use the org.jenkins.ui.icon package?

[timja]

LGTM.

What could be the impact on plugins that import the icon-set library, such as jenkinsci/parameterized-trigger-plugin or jenkinsci/authorize-project-plugin

Search link: github.com/search?l=Maven+POM&q=org%3Ajenkinsci+icon-set&type=Code

To clarify, what happens with plugins that import icon-set and use the org.jenkins.ui.icon package?

I think it will work fine but needs testing, alternatively we just remove it from those libs as I can't see it still being needed, / we do both.

[fqueiruga]

Sounds good to me. I don't have a good understanding of classpaths or the workings of the JVM so I wasn't sure.

[jglick]

https://github.com/jenkinsci/parameterized-trigger-plugin/blob/02ee6ea56ed988b824f341945d40ba5ab162f77f/pom.xml#L123-L126 is an exclusion. Most of the other references also seem to be bogus or obsolete, just POM wrangling. https://github.com/jenkinsci/node-sharing-plugin/blob/adf77d8f191e0dd321fb0d493c308868d545047d/pom.xml#L125-L129 is an actual dependency—which is useless, AFAIK, since the jenkins.version is > 2.0. I think both the plugin and the library it wraps can be considered deprecated and the repository archived.

[jglick]

timja added internal skip-changelog labels 6 hours ago

Well…should be listed in release notes that the plugin and library are obsolete so if your plugin has any reference to either, it can be removed now (and probably could have been removed before).

[jglick]

https://github.com/jenkinsci/jenkins/pull/5072/checks?check_run_id=1441456689

[timja]

#5072 (checks)

resolved, icon-shim plugin 2.0.2 had breaking changes (and nothing resembling a useful changelog in that repo 😂 ), this is the lovely commit jenkinsci/icon-shim-plugin@1d35827

It was never released in Jenkins core, I've downgraded to what core was previously using and it fixes the issue.

Weirdly I couldn't get this test to run locally I get issues with the user not being created properly.
I ended up running it interactively and the issue was quite clear.

[timja]

This PR is now ready for merge, after ~24 hours, we will merge it if there's no negative feedback.

Thanks!

stale

[jtnord]

@timja does this not need a corresponding change in https://github.com/jenkinsci/icon-shim-plugin to remove the classes and bump the plugin baseline to a version of jenkins with this changes?

anyone with a plugin first classloader or other funky things may end up getting one version of these classes, whereas a different plugin (or core) can get another one, leading to some funky errors?

[timja]

Hmm, we did a quick search and found very few uses of it so it seemed low risk:
https://github.com/search?l=Maven+POM&q=org%3Ajenkinsci+icon-shim&type=Code

How would updating icon-shim help as people would still be using the old version wouldn't they?

[jglick]

I think the idea was to delete https://github.com/jenkinsci/icon-shim-plugin/blob/6df80f4fcf8a4eb17e4c955e7e941beee519a6da/plugin/pom.xml#L44-L48, cut a “tombstone” plugin release with that change, then archive the repo.

[timja]

on it

[jglick]

Not sure about details because I am not very familiar with this. If doing anything, be careful with test in real java -jar jenkins.war mode, not hpi:run or the like.