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

Add phase to animal-sniffer to fix mvn test #258

Merged
merged 1 commit into from
Nov 25, 2019

Conversation

timja
Copy link
Member

@timja timja commented Nov 21, 2019

Without this change I couldn't run mvn test in the https://github.com/jenkinsci/configuration-as-code-plugin

The error was:

animal-sniffer-maven-plugin:1.17:check: java.lang.NoSuchMethodError: java.nio.CharBuffer.position(I)Ljava/nio/CharBuffer;

the documentation says to use test https://www.mojohaus.org/animal-sniffer/animal-sniffer-maven-plugin/usage.html

cc @oleg-nenashev @sladyn98 @Casz

@sladyn98
Copy link

sladyn98 commented Nov 21, 2019

One of the fixes suggested on a forum was to use v 1.18 but that didn't work either. This should sort it

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.

Fine with me

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

LGTM maybe upgrade to last version of the plugin as well?
and add a comment TODO to remove this and use --release when using java9+ :) (maybe we could have some profile for that? but it's a bigger change..)

@oleg-nenashev
Copy link
Member

LGTM maybe upgrade to last version of the plugin as well?

Are there newer ones? Dependabot should have proposed them

and add a comment TODO to remove this and use --release when using java9+ :) (maybe we could have some profile for that? but it's a bigger change..)

Ongoing work in #133 . I moved on to other stories due to the priority change, but it can be finalized.

@olamy
Copy link
Member

olamy commented Nov 22, 2019

1.18 release date is 2019-05-12

@jglick
Copy link
Member

jglick commented Nov 22, 2019

What was the problem here—why was only this plugin affected?

@sladyn98
Copy link

I am not quite sure but seems like version 1.17 was not compatible with java 8.

@oleg-nenashev
Copy link
Member

FTR #200 . Dependabot said "Looks like these dependencies are up-to-date now, so this is no longer needed.". It is a follow-up to the #229 revert last week which updated the Animal Sniffer. The Dependabot just has not resubmitted the PR yet

@timja
Copy link
Member Author

timja commented Nov 22, 2019

What was the problem here—why was only this plugin affected?

mvn install worked but mvn test didn't, I was specifically trying to reproduce an issue which didn't occur when running in intellij but was unable to run a single test

(error is in PR description)

@jglick
Copy link
Member

jglick commented Nov 22, 2019

So this is thought to be a regression from a dependency update? Needs an IT.

@oleg-nenashev
Copy link
Member

Merge it anyway? Just would like to cut a new release to reintroduce BOM

@oleg-nenashev
Copy link
Member

One of the fixes suggested on a forum was to use v 1.18 but that didn't work either. This should sort it

1.18 just got merged in #257 . If the problem persists after it, i will cut a release for this fix also

@timja
Copy link
Member Author

timja commented Nov 25, 2019

One of the fixes suggested on a forum was to use v 1.18 but that didn't work either. This should sort it

1.18 just got merged in #257 . If the problem persists after it, i will cut a release for this fix also

the problem is there with both versions, I tested upgrading when I was troubleshooting and it didn't help any without this fix

@oleg-nenashev
Copy link
Member

OK, let's merge it then

@oleg-nenashev oleg-nenashev merged commit f303269 into jenkinsci:master Nov 25, 2019
@timja timja deleted the fix-mvn-test branch November 25, 2019 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants