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

Fix mvn variable name for checkstyle version #586

Closed
romani opened this issue Jan 29, 2022 · 13 comments
Closed

Fix mvn variable name for checkstyle version #586

romani opened this issue Jan 29, 2022 · 13 comments

Comments

@romani
Copy link
Contributor

romani commented Jan 29, 2022

Is your feature request related to a problem? Please describe.
Checkstyle CI is failing:
https://app.travis-ci.com/github/checkstyle/checkstyle/jobs/557586875#L545

Describe the solution you'd like
8d9703e#r65114830

Describe alternatives you've considered
NA

Additional context
in checkstyle repo:
mvn -e --no-transfer-progress clean install -Pno-validations
./.ci/validation.sh no-error-equalsverifier
should pass.

romani added a commit to checkstyle/checkstyle that referenced this issue Jan 29, 2022
@romani
Copy link
Contributor Author

romani commented Jan 29, 2022

as you fix names, you can send us PR to enable your project in CI regression testing.

checkstyle/checkstyle@2372df1

MUzairS15 pushed a commit to MUzairS15/checkstyle that referenced this issue Jan 30, 2022
@jqno
Copy link
Owner

jqno commented Jan 30, 2022

Thanks for the heads up about this! Can you show me what exactly fails?

I did change names but the behavior from your side should be unaffected. Of course it's possible I made a mistake, but I don't know what it is.

Another change I made is the fact that all static analysis, including checkstyle, now only triggers on JDK17. Could that be the problem? What JDK does your CI use?

Anyway, I'll have some time to make a PR tomorrow.

@romani
Copy link
Contributor Author

romani commented Jan 30, 2022

First link in issue description is link to full log of execution.

In commit to disable, you can see that we used jdk8, good to know that it should be jdk17

@jqno
Copy link
Owner

jqno commented Jan 30, 2022

Ah ok, if I can just specify JDK17, that will be an easy fix. I'll also add a comment in my pom file to send you a PR if I ever update that to a newer JDK (probably when the next LTS rolls around). I don't know yet if Travis support JDK 17 yet though. I think I moved away from them because of slow adoption, but I'm not sure anymore.

Otherwise, I'll create a Maven profile for checkstyle.

@romani
Copy link
Contributor Author

romani commented Jan 30, 2022

Please look at referenced PR above, Travis support jdk17, but we need variable to work.

@jqno
Copy link
Owner

jqno commented Jan 31, 2022

I have created a PR to update the pom variable: checkstyle/checkstyle#11258

I took a look at the JDK17 PR, it seems to fail on something other than the pom variable. I'm totally willing and able to put checkstyle in a separate Maven profile so that it can run on any JDK version. I'll also update the PR to incorporate that into your scripts, if you want. Just say the word!

@romani
Copy link
Contributor Author

romani commented Feb 1, 2022

no problem , do what you think is good, and just let us know (send PR) how we should run it.

@romani
Copy link
Contributor Author

romani commented Feb 1, 2022

we are migrating now (I hope to be done in next few days) from jdk8 to jdk11, so the most convenient for us is openjdk11.
But it is regression Ci execution, we can run even in jdk17 (we are build-able on this jdk too) unfortunately Travis does not have openjdk17 support for now on free tier. So execution on jdk11 is preferable (we can try jdk16 it is declared to be supported).

Most ideal for us case is execution of checkstyle without any other plugins/tools or validation to let us test only checkstyle and avoid any other validation problems.

@romani romani changed the title Fix mavn variable name for checkstyle version Fix mvn variable name for checkstyle version Feb 1, 2022
@romani
Copy link
Contributor Author

romani commented Feb 1, 2022

I made pass by usage of profile and extra hack in your pom
https://github.com/checkstyle/checkstyle/pull/11258/files#diff-a74bb028b327d2ef179514069ca703e16a92ca5d0a18670c2d3daa356e032d7cR727

but better to make it less hacky. you might can use some variables to choose args based on jdk.
openjdk11 does not have removal parameter
so build is passing even by sed -i'' 's/,removal,/,/' pom.xml

@jqno
Copy link
Owner

jqno commented Feb 1, 2022

Haha, nice use of sed 😄
I will add a profile to the pom containing only checkstyle. That way, you don't have to hack around it, and I don't have a sneaky indirect dependency on whatever Java versions TravisCI supports. When I've done that and tested it a bit, I'll update checkstyle/checkstyle#11258 .
Hopefully I can do that tomorrow.
Or, if you're fine with the way things are now, just merge the PR and I'll open up a new one when I'm ready.

@jqno
Copy link
Owner

jqno commented Feb 2, 2022

I've created a profile and updated the PR.

@romani
Copy link
Contributor Author

romani commented Feb 3, 2022

fixed in Checkstyle repo.
@jqno , thanks a lot for help.

@romani romani closed this as completed Feb 3, 2022
@jqno
Copy link
Owner

jqno commented Feb 4, 2022

And thank you for your patience, @romani . After all, I'm the one who completely changed my build without thinking about the consequences for you rproject :).

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

No branches or pull requests

2 participants