Skip to content

[JENKINS-34466] Fix PCT against 2.0#63

Merged
jglick merged 2 commits intojenkinsci:masterfrom
andresrc:JENKINS-34466
May 11, 2016
Merged

[JENKINS-34466] Fix PCT against 2.0#63
jglick merged 2 commits intojenkinsci:masterfrom
andresrc:JENKINS-34466

Conversation

@andresrc
Copy link

@andresrc andresrc commented Apr 27, 2016

When building against Jenkins 2.0 Groovy 2.4.6 is used instead of 1.8.9 in the baseline. This brings a couple of issues:

  • For some methods in the whitelist there are additional overloads for more specific type arguments, generating approval errors.
  • The behavior for ambiguous overloading has changed: no longer "some" version of the method is picked but an exception is thrown.

Besides, the PR includes:

@reviewbybees

staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods collect java.lang.Object groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods collect java.util.Collection groovy.lang.Closure
staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods disjoint java.util.Collection java.util.Collection
g2 staticMethod org.codehaus.groovy.runtime.DefaultGroovyMethods each java.lang.Iterable groovy.lang.Closure
Copy link
Author

Choose a reason for hiding this comment

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

New overloads in Groovy 2.4.6 not in 1.8.9

Copy link
Member

Choose a reason for hiding this comment

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

🐛 this is not maintainable, and there are/will be other libraries with different versions (including Jenkins core). I suppose the only reason not to just add the signature with no prefix is so that GenericWhitelistTest.sanity does not complain? Then just add a separate generic-whitelist-groovy2 file and do not include it in sanity (or include it only if we can detect that the Groovy version in the classpath is sufficiently new).

Copy link
Member

Choose a reason for hiding this comment

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

(treating as a bug not an ant because this is a format change and thus not easily reverted)

// Cannot use until approved
create().assertCannotUse().approve().use();
}

Copy link
Author

Choose a reason for hiding this comment

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

Cleanup from #53

@ghost
Copy link

ghost commented Apr 27, 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.

@armfergom
Copy link

This LGTM as far as I understand. 🐝

*/
public final class StaticWhitelist extends EnumeratingWhitelist {
private static final String G2_PREFIX = "g2";
private static final boolean GROOVY2 = GroovySystem.getVersion().startsWith("2.");
Copy link
Member

Choose a reason for hiding this comment

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

🐜 I think you want to use VersionNumber or similar to apply the same behavior (unless and until overridden) in Groovy 3.x.

* Whitelist based on a static file.
*/
public final class StaticWhitelist extends EnumeratingWhitelist {
static final boolean GROOVY2 = new VersionNumber(GroovySystem.getVersion()).compareTo(new VersionNumber("2")) >= 0;
Copy link
Member

Choose a reason for hiding this comment

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

🐜 inline into GenericWhitelistTest, its only remaining usage.

@jglick
Copy link
Member

jglick commented May 10, 2016

🐝

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.

3 participants