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

Revert symbol-annotation bump #5293

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Conversation

jglick
Copy link
Member

@jglick jglick commented Feb 17, 2021

Reverts #5265. Noticed in jenkinsci/file-parameters-plugin#27 that this caused a build failure

[WARNING] Rule 5: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for org.jenkins-ci:symbol-annotation:1.20 paths to dependency are:
+-io.jenkins.plugins:file-parameters:98.728b4f423129
  +-org.jenkins-ci.plugins:structs:1.20
    +-org.jenkins-ci:symbol-annotation:1.20
and
+-io.jenkins.plugins:file-parameters:98.728b4f423129
  +-org.jenkins-ci.main:jenkins-core:2.281-rc30899.58174df06127
    +-org.jenkins-ci:symbol-annotation:1.20 (managed) <-- org.jenkins-ci:symbol-annotation:1.21
]

This is the most straightforward way to fix the immediate regression. There are probably other workarounds, though not using optional—we do want it bundled in the WAR! Really the root of the evil is that we have this JAR which is both a core library dep and bundled in a plugin. I do not recall why things were set up this way exactly; probably to avoid needing to use new core deps in plugins, at a time when that seemed really scary.

Proposed changelog entries

  • Developer: Downgrade the bundled symbol-annotation library version to prevent build failures in plugins

@@ -518,7 +518,7 @@ THE SOFTWARE.
<dependency> <!-- Not included into BOM, plugins should use one from structs-plugin -->
<groupId>org.jenkins-ci</groupId>
<artifactId>symbol-annotation</artifactId>
<version>1.21</version>
<version>1.1</version>
Copy link
Member

Choose a reason for hiding this comment

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

why not just bump the version of structs? you just need to use bom version 24.

I noticed this in jenkinsci/plot-plugin#65, but bumping bom fixed it

Copy link
Member Author

Choose a reason for hiding this comment

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

The particular version does not matter; it has not changed since 1.0 AFAIK. I just do not want it to be constantly bumped up here.

Copy link
Member

Choose a reason for hiding this comment

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

Which one is actually used at runtime? the structs one or the Jenkins core one?

Copy link
Member Author

Choose a reason for hiding this comment

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

The Jenkins core one. Which does not really matter because

git diff structs-parent-1.1 -- annotation/src/main
diff --git annotation/src/main/java/org/jenkinsci/Symbol.java annotation/src/main/java/org/jenkinsci/Symbol.java
index abd45ef..7f007e2 100644
--- annotation/src/main/java/org/jenkinsci/Symbol.java
+++ annotation/src/main/java/org/jenkinsci/Symbol.java
@@ -33,6 +33,10 @@ import static java.lang.annotation.RetentionPolicy.*;
  * The first one is used as the primary identifier for reverse-mapping.
  *
  * <p>
+ * Generally symbols should be named after their display names, so that users see
+ * association between what they see in web UI and symbols they see in code.
+ *
+ * <p>
  * To look up a component by its symbol, see the documentation of the symbol plugin.
  */
 @Indexed

Copy link
Member

Choose a reason for hiding this comment

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

The last code change was in 1.0, so it does not matter much: jenkinsci/structs-plugin@2efc80d#diff-94e7f2d13ab308fa2df75a0097ab64be6d274e2a8233741ca4c45f1195602c71 . The produced bytecode is likely to be different between versions though.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why structs doesn't just pickup the jenkins core one?

Shouldn't we just remove the dependency there, what if we did want to update the library?

Copy link
Member Author

Choose a reason for hiding this comment

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

why structs doesn't just pickup the jenkins core one

It could, if the annotation component were split to its own repo with its own lifecycle.

Copy link
Member

Choose a reason for hiding this comment

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

It could, if the annotation component were split to its own repo with its own lifecycle.

@timja @jglick I extracted this to a separate repository here: https://github.com/basil/lib-symbol-annotation

What are the next steps? Get this hosted under the jenkinsci GitHub organization, add myself to RPU and cut a release, and then update core to use the new release and structs plugin to use the version from core? Can the former be sorted casually or shall I file a HOSTING request?

Copy link
Member

Choose a reason for hiding this comment

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

Can do you a hosting request please.

Copy link
Member

Choose a reason for hiding this comment

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

Done: HOSTING-1155

@timja
Copy link
Member

timja commented Feb 17, 2021

This is the most straightforward way to fix the immediate regression. There are probably other workarounds, though not using optional—we do want it bundled in the WAR! Really the root of the evil is that we have this JAR which is both a core library dep and bundled in a plugin. I do not recall why things were set up this way exactly; probably to avoid needing to use new core deps in plugins, at a time when that seemed really scary.

Could we remove it from the structs plugin?

@jglick
Copy link
Member Author

jglick commented Feb 17, 2021

Could we remove it from the structs plugin?

Yes, the proper fix would be to split to an independent repo, remove it as a dep from structs, and just expect it to be picked up from jenkins-core like any other bundled lib (managed in jenkins-bom). This patch is more of a hotfix.

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.

When I reviewed changes in the sources that contribute to symbol-annotation.jar, they had not changed since at least version 1.3 and it may have been there were no changes since even before that.

No problem for me if we hold this at version 1.1.

@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.

I've seen the same problem in platformlabeler plugin builds that @jglick reported in jenkinsci/file-parameters-plugin#27

jglick added a commit to jglick/file-parameters-plugin that referenced this pull request Feb 18, 2021
@daniel-beck daniel-beck added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Feb 21, 2021
@daniel-beck
Copy link
Member

Mark commented but didn't label, doing so now.

@MarkEWaite
Copy link
Contributor

@daniel-beck I think my listing it as ready to merge was premature. I believe we still need a second approval before it is merged.

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.

Reasonable short-term fix at least

@oleg-nenashev oleg-nenashev added regression-fix Pull request that fixes a regression in one of the previous Jenkins releases bug For changelog: Minor bug. Will be listed after features developer Changes which impact plugin developers labels Feb 22, 2021
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.

Should be good to go

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For changelog: Minor bug. Will be listed after features 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 regression-fix Pull request that fixes a regression in one of the previous Jenkins releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants