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-39414] - Workaround Ruby Runtime Plugin and Jenkins 2.28 compatibility issues #85

Merged
merged 2 commits into from
Nov 4, 2016

Conversation

oleg-nenashev
Copy link
Member

@oleg-nenashev oleg-nenashev commented Nov 2, 2016

Stapler 1.266 was formally compatible if we take a set of modules, but we didn't take into account that components like stapler-jruby are actually standalone libs being bundled in plugins like Ruby Runtime.

Verifies there is no regression in the Jenkins core itself.

  • Add direct unit tests for Field processing for common and Ruby classes
  • Suppress error when a plugin uses obsolete lib (e.g. stapler-ruby in Ruby Runtime)

Error suppression is a weird approach, but I cannot offer anything better in the current state. A long-term fix would be to rework Stapler multi-module structure and to somehow ensure that entire stapler code gets delivered via something like core module

https://issues.jenkins-ci.org/browse/JENKINS-39414

@reviewbybees

@ghost
Copy link

ghost commented Nov 2, 2016

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

…Fields() when a Jenkins plugin uses obsolete Stapler lib
@oleg-nenashev oleg-nenashev changed the title [JENKINS-39414] - Add direct unit tests for Field processing for common and Ruby classes [JENKINS-39414] - Workaround Ruby Runtime Plugin and Jenkins 2.28 compatibility issues Nov 2, 2016
return navigator.getDeclaredFields(clazz);
try {
return navigator.getDeclaredFields(clazz);
} catch (AbstractMethodError err) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually this handling supresses error I've expected to catch in tests, but tests on aligned stapler module versions pass independently of this hack

@oleg-nenashev
Copy link
Member Author

Likely https://github.com/jenkinsci/jenkins.rb needs a similar fix

return navigator.getDeclaredFields(clazz);
} catch (AbstractMethodError err) {
// A plugin uses obsolete version of Stapler-dependent library (e.g. JRuby), which does not offer the method (JENKINS-39414)
// TODO: what to do with Logging? The error must be VERY visible, but it will totally pollute system logs
Copy link
Member

Choose a reason for hiding this comment

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

log every 100 occurrence?

Copy link
Member

Choose a reason for hiding this comment

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

OR just FINE log 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.

Both options do not work since we may invoke this method dozens and dozens of times during serving a single page. I would rather trigger administrative monitor in Jenkins, but it requires something (e.g. publicly accessible static field with a list of failed navigators). A bit hackish...

@rsandell
Copy link
Member

rsandell commented Nov 2, 2016

🐝

@vivek
Copy link
Contributor

vivek commented Nov 2, 2016

🐝

@oleg-nenashev
Copy link
Member Author

@reviewbybees done

@ghost
Copy link

ghost commented Nov 3, 2016

This pull request has completed our internal processes and we now respectfully request the maintainers of this repository to consider our proposal contained within this pull request for merging.

@jglick
Copy link
Member

jglick commented Nov 3, 2016

🐝

Actually I think stapler-jruby should be a separate repository with its own lifecycle, and we just strive to maintain binary compatibility.

@oleg-nenashev
Copy link
Member Author

Actually I think stapler-jruby should be a separate repository with its own lifecycle, and we just strive to maintain binary compatibility.

Yes, it's a common pitfall of Maven multi-module projects. Actually there are many repositories to be decoupled if we follow such approach.

@oleg-nenashev
Copy link
Member Author

@kohsuke @jglick @rsandell @daniel-beck Ping. We need a release ASAP.

@rsandell rsandell merged commit b9477c9 into jenkinsci:master Nov 4, 2016
@rsandell
Copy link
Member

rsandell commented Nov 4, 2016

I have merged, but only @kohsuke can release as it needs to be signed by his key iirc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants