[JENKINS-39414] - Stapler 1.246 Binary Incompatibility. Part 2 #88
Conversation
Required at least for tests, seems to be useful in general
…ns in KlassNavigator
|
🐝 |
| // TODO: what to do with Logging? The error must be VERY visible, but it will totally pollute system logs | ||
| return Collections.emptyList(); | ||
| } | ||
| return navigator.getDeclaredFields(clazz); |
There was a problem hiding this comment.
IMHO we need to test this change. You removed the try catch, but are sure that getDeclaredFields never throws an exception?
There was a problem hiding this comment.
It was a hack in #85 in order to suppress the binary compatibility issue. Now we solve it by providing default implementations in KlassNavigator. Hence this AbstractMethodError handler is no longer required
|
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. |
|
@oleg-nenashev ok thanks for the clarification 🐝 |
|
@reviewbybees done |
|
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. |
| * @param clazz Class | ||
| * @return List of the fields functions declared for the class. | ||
| * By default this list is empty, {@link KlassNavigator} implementations are responsible to implement it. | ||
| * @since 1.246 |
There was a problem hiding this comment.
BTW getDeclaredMethods was also added as abstract somewhat recently (1.220), but I suppose if this caused problems we would have hit them long since.
There was a problem hiding this comment.
Yes, I decided not to touch that code. Ruby Runtime has a dep on a greater version
| * @since 1.248 | ||
| */ | ||
| @CheckForNull | ||
| public String getName() { |
There was a problem hiding this comment.
Is there a proposed caller for this, or is it just KlassTest?
There was a problem hiding this comment.
I think this API is generally useful, e.g. for diagnostic purposes. Right now just in KlassTest and RubyKlassNavigatorTest. The latter one is supposed to be an external lib.
| MyRubyClass(Ruby runtime) { | ||
| super(runtime); | ||
| } | ||
| } |
|
FTR this corrects a regression introduced in #77. jenkinsci/ruby-runtime-plugin#4 or its port to |
1.247 didn't fix the issue completely, because there was another incompatible method (
getFunctions()), which I have missed somehow.This change intends to solve the binary compat issue:
KlassNavigatormethods introduced in 1.246KlassNavigator#getFunctions()KlassNavigator#getDEclaredMethods()(generally was not required for this PR)I've verified that Ruby Runtime plugin starts correctly and shows UIs without errors in System logs after the fix.
https://issues.jenkins-ci.org/browse/JENKINS-39414
@reviewbybees