Issue #7190: remove deprecated setClassLoader methods by nrmancuso · Pull Request #7778 · checkstyle/checkstyle

nrmancuso · 2020-03-03T20:34:15Z

Issue #7190: remove deprecated setClassLoader methods

romani · 2020-03-04T12:54:31Z

Same problem in pgjdbc project Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check

Did you send PR to them ?

rnveach · 2020-03-04T13:08:45Z

romani · 2020-03-04T16:15:19Z

New failure org.apache.maven.plugins:maven-checkstyle-plugin:3.1.0:check (default-cli) on project orekit: no surprise :).

@nmancus1 , please review at https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh all execution points with prefix "no-error" and create PR for such projects.

rnveach · 2020-03-04T16:25:53Z

@nmancus1 When providing this update PRs to external groups, please add some context to the PR information so they know why we need this change accepted.

See some of my previous PRs as examples:
pgjdbc/pgjdbc#904
pgjdbc/pgjdbc#1434

nrmancuso · 2020-03-04T16:53:16Z

@romani I'm on it.
@rnveach Thanks for the pointers. I will do a more thorough job in the future when submitting PR's to other projects.

nrmancuso · 2020-03-04T20:36:55Z

@rnveach How can I generate the failing regression locally, so that I can provide that output in the PRs I'm submitting (similar to your example)?

PR's sent( I'll update as I make them ):
xwiki: xwiki/xwiki-commons#88
apex-core: https://issues.apache.org/jira/browse/APEXCORE-820
equalsverifier(already merged, commited by dependabot): jqno/equalsverifier@a8d7ad4
orekit: https://gitlab.orekit.org/orekit/orekit/-/merge_requests/50
Sevuntu-checks: sevntu-checkstyle/sevntu.checkstyle#804
Hibernate-Search: hibernate/hibernate-search#2238

romani · 2020-03-05T14:25:04Z

CI is simply execute single command line that we can execute on linux local, example -

checkstyle/wercker.yml

Line 88 in c7ae67e

./.ci/wercker.sh no-error-xwiki

same commands should work on your linux local laptop, execution if from repo root directory.

romani · 2020-03-05T14:38:40Z

@nmancus1 , please remove from commit not your changes.
Please resolve all conflicts and .... . Looks we are waiting for xwiki only .

apex-core did not update for 3 years, according to github https://github.com/apache/apex-core , I did a fork https://github.com/checkstyle/apex-core , @nmancus1 , please send PR to our fork, please update https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L103 to use checkstyle's fork.
We might remove this project from testing as it is looks like dead.

nrmancuso · 2020-03-05T15:59:12Z

CI is simply execute single command line that we can execute on linux local, example -

I'm lazy, I wanted to run all of the commands via script, without writing a new one :)

Please resolve all conflicts and .... . Looks we are waiting for xwiki only .

apex-core did not update for 3 years, according to github https://github.com/apache/apex-core , I did a fork https://github.com/checkstyle/apex-core , @nmancus1 , please send PR to our fork, please update https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L103 to use checkstyle's fork.
We might remove this project from testing as it is looks like dead.

I'll get this fixed up this today.

romani · 2020-03-05T16:09:33Z

I wanted to run all of the commands via script,

Just copy single command from CI config, like
./.ci/wercker.sh no-error-xwiki paste it to Linux terminal, and you will have the same result. It is applicable for all CIs.

rnveach · 2020-03-05T17:44:49Z

I wanted to run all of the commands via script, without writing a new one :)

I understand this and want something similar so I can execute same CI commands locally on my checkstyle VM server which I use for regression reports. The issue is going to be keeping it in-synch with the YMLs as CI can change regulary.

For now, you will have to create such a script manually. I would move this type of discussion to a new issue if you want to continue to add it to checkstyle in some form.

nrmancuso · 2020-03-06T04:14:22Z

@romani I have ran all of the wercker scripts locally, and none are failing for me, but CI still fails when I push. Can you let me know what is failing since I can't view the logs?

romani · 2020-03-06T05:45:15Z

Wercker is failing on orekit

Command: ./.ci/wercker.sh no-error-orekit
CS_version: 8.31-SNAPSHOT
Cloning into 'hipparchus'...
Note: checking out '4c6c6fc45e859eae2d4eb091a3a3c0a7a458b8d9'.

You are in 'detached HEAD' state. You can look around, make experimental
changes and commit them, and you can discard any commits you make in this
state without impacting any branches by performing another checkout.

If you want to create a new branch to retain commits you create, you may
do so (now or later) by using -b with the checkout command again. Example:

We need to update checkout commit in wercker.sh.
Please send PR to our apex fork.

rnveach · 2020-03-06T10:50:27Z

I have ran all of the wercker scripts locally, and none are failing for me, but CI still fails

Without seeing what you did,

Wercker is special in that it has a pre-execution command before each command run.

https://github.com/checkstyle/checkstyle/blob/master/wercker.yml#L58
You must run install on your current checkstyle branch that is this PR and then run each SH command from the same directory using the commands in the rest of the YML.

nrmancuso · 2020-03-07T03:18:38Z

Please send PR to our apex fork.

@romani I am struggling to get apex past mvn clean verify so that I can send PR. Lots of failing tests. Where can we discuss?

We need to update checkout commit in wercker.sh.

Should I submit a PR for this as 'minor"?

rnveach · 2020-03-07T12:23:22Z

I am struggling to get apex past mvn clean verify

Verify will run the tests of the repo. For regression, we don't want any tests being run. Please look at the original command used for apex. https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L105

nrmancuso · 2020-03-07T12:30:12Z

Verify will run the tests of the repo. For regression, we don't want any tests being run. Please look at the original command used for apex.

So, just to be clear, it is ok to submit a PR to checkstyle's apex repo that will not pass mvn clean verify? The contribution rules still link to the old Apache site, so I thought it best to be thorough.

rnveach · 2020-03-07T12:36:52Z

It is the 3 year old project @romani mentioned. If they have failing tests on a clean master, then you would be wasting your time on fixing a project that is not even maintained by the owners. If it was an active project, yes, the PR sent to them should pass their CI and we could ask their maintainers for their help resolving any CI issues. None of us know their project in any details to know how to help. From our end, these projects are only used for regression. We just need to make sure we have a command to run regression on them and ensure a stable build, only for regression purposes. We always skip tests on these projects to speed up execution time and because we are not involved in testing their own project. Just their checkstyle portion.

nrmancuso · 2020-03-07T12:46:15Z

Ok. Thanks for explaining.

romani · 2020-03-07T16:44:30Z

@nmancus1 ,
please review code at https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L62
there are two checkouts to certain SHAs, please update second to any SHA from orekit repo after your fix is merged, first SHA is related to dependeing library, any recent SHA will be ok.

apex PR is merged - https://github.com/checkstyle/apex-core/pull/1
so please update https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L103 to use our fork instead of original repo.

nrmancuso · 2020-03-07T20:26:32Z

so please update https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L103 to use our fork instead of original repo.

PR is here: #7786

rnveach · 2020-03-07T22:37:31Z

orekit: https://gitlab.orekit.org/orekit/orekit/-/merge_requests/50

Orekit was merged, but we specific commit in regression because of their unstable CI.
See https://github.com/checkstyle/checkstyle/blob/master/.ci/wercker.sh#L78

This needs to be updated for the CI in this PR to pass.

nrmancuso · 2020-03-07T22:42:36Z

@rnveach I just submitted pull request for updated Orekit commit at #7793

rnveach · 2020-03-11T01:55:36Z

./.ci/wercker.sh no-error-methods-distance
All of these build for me.

Then why does methods distance in wercker fail, which looks because its maven-checkstyle-plugin was not updated to the newest version?
https://github.com/sevntu-checkstyle/methods-distance/blob/master/pom.xml#L21

nrmancuso · 2020-03-11T02:08:15Z

Then why does methods distance in wercker fail, which looks because its maven-checkstyle-plugin was not updated to the newest version?

I failed to clean install. After doing that, I can reproduce this error. I'll submit PR now, and recheck the rest. My apologies.

rnveach · 2020-03-11T02:10:44Z

Please reconfirm the others with the clean install too.

nrmancuso · 2020-03-11T02:46:28Z

@rnveach ./.ci/wercker.sh no-error-contribution is the only other one failing. What can I do to fix this, considering the variable used here: https://github.com/checkstyle/contribution/blob/fdc04f5789658cf002832c6eb5ea33ccd1c5b09a/checkstyle-tester/pom.xml#L65 ?

Can we add arguments to maven to specify the correct plugin version?

romani · 2020-03-11T03:44:57Z

Wercker is restarted, Travis is failing on spelling from Sha commit value.

rnveach · 2020-03-11T10:42:35Z

./.ci/wercker.sh no-error-contribution is the only other one failing. What can I do to fix this, considering the variable used here: https://github.com/checkstyle/contribution/blob/fdc04f5789658cf002832c6eb5ea33ccd1c5b09a/checkstyle-tester/pom.xml#L65 ?

I am not seeing your issue. Why can't you update the version number at https://github.com/checkstyle/contribution/blob/fdc04f5789658cf002832c6eb5ea33ccd1c5b09a/checkstyle-tester/pom.xml#L14 ?
Please expand on the issue more.

nrmancuso · 2020-03-11T11:36:08Z

Please expand on the issue more.

Lack of sleep and inexperience. Thanks for pointing that out. PR submitted.

nrmancuso · 2020-03-11T11:47:36Z

What should I do about htmlunit: #7778 (comment) ? I thought it would be easier to move to GitHub, but if that complicates things, I'll submit a PR over there, too, and stick with SVN.

rnveach · 2020-03-11T12:09:36Z

Update to contribution merged. Wecker doesn't need a restart as it hasn't started yet.

rnveach · 2020-03-11T12:11:55Z

I thought it would be easier to move to GitHub, but if that complicates things, I'll submit a PR over there, too, and stick with SVN.

Have they fully switched to Github and put SVN in a read only state? If so, switch to Github should be in this PR unless @romani has a complaint since it now seems it is directly tied to their maven version.

Either this PR or another, you are relied on this change for this PR.

rnveach · 2020-03-11T12:14:47Z

minor: remove hipparchus temp directory

This doesn't look like it is connected to this issue. If so, move this to another PR so it can be merged faster.

romani

wercker is passed.
I am approving this PR, could be merged when all CIs green.

@rnveach , please finalize.

rnveach · 2020-03-11T14:20:59Z

.ci/wercker.sh

-  mvn -e compile checkstyle:check -Dcheckstyle.version=${CS_POM_VERSION}
+  checkout_from https://github.com/HtmlUnit/htmlunit
+  cd .ci-temp/htmlunit
+  mvn -e compile -DskipTests=true -Dcheckstyle.version=${CS_POM_VERSION}


This command is incomplete. We need more than a compile. Please see the pervious command and commands from other places. I don't see why actual command would change since they just moved from SVN to Git.

nrmancuso · 2020-03-11T18:52:01Z

Have they fully switched to Github and put SVN in a read only state?

It appears so: https://github.com/HtmlUnit/htmlunit/releases/tag/2.33

I'm not sure how to determine if their SVN is in a read only state, but there hasn't been any commits to SVN since 2018 that I can see.

I have reverted 599231b . I need to uncomment this block of script, right? #7778 (comment). What else do I need to take care of so that this can get merged? Do you want me to squash all commits besides ef07590 and 09f466b?

rnveach · 2020-03-11T20:43:31Z

there hasn't been any commits to SVN since 2018 that I can see.

Since its been 2 years and if the Git repository is still active, then it seems highly likely that SVN isn't used anymore and so the commit must remain here. If it is not literally read only, then if usage seems that way, then it is good enough for me. They won't push a commit to it after 2 years of abandoning it.

What else do I need to take care of so that this can get merged?

#7778 (comment)
Move this commit to another PR unless you can tell me how it is connected to this issue. We should not see a revert commit or anything left of this change in this PR. When moved to other PR, it can be marked as a "minor" change.

#7778 (comment)
Resolve this item. This one looks like blocking problem.

Any commits remaining that are attached to this issue must have the issue in the commit message. IE: Issue #7190:

Make CI happy. We will only merge when CI is green. I am not talking about random failures like timeout issues which we see happen from time to time. I am talking full on errors, violations, etc...

Don't forget to rebase on latest master.

Do you want me to squash all commits besides ef07590 and 09f466b?

No. Leave them all separate.

rnveach · 2020-03-11T21:16:37Z

.ci/wercker.sh

+  checkout_from https://github.com/HtmlUnit/htmlunit
+  cd .ci-temp/htmlunit
  mvn -e compile checkstyle:check -Dcheckstyle.version=${CS_POM_VERSION}
  cd ../


cd changed from cd htmlunit to cd .ci-temp/htmlunit. Since we are cding into a second directory, don't we need to cd out an additional directory too?

Actually, we are deleting the htmlunit folder so we should only go out 1. Please double confirm. I am seeing out projects with a double cd and I think atleast 1 might be wrong.

What I have is correct, there are other errors in this file, like what you've mentioned. There are a few that do that, but they end up living in .ci-temp and never getting deleted, just updated, which I guess could be either good or bad, but the intended action of rm'ing the directories isn't carried out.

Would you like me to tidy this file up along with the hipparchus directory removal and submit as one PR?

Yes, feel free to make another PR ensure the temp folder is tidied up. Not in this PR. I was asking @romani since he knows linux better than me: can we remove the f option for rm -rf but not make it prompt? This way it will complain if directory doesn't exist and catch this problem for us.
http://man7.org/linux/man-pages/man1/rm.1.html

In my experience (I have much more with Linux than with programming), only the -i flag will make it prompt when rm'ing, despite what the man page says. Since it is a git repo though, it might be necessary to use -f because of the protected files. I'll take a look tonight.

Let's move all noticed points to improve to separate issue/PRs .

It is better to merge this PR sooner.

@nmancus1 Do you still plan to do this?

@rnveach yes, I completed it today. I just wanted to run through and test each one again before submitting PR.

nrmancuso mentioned this pull request Mar 3, 2020

Issue #7190: remove deprecated setClassLoader methods #7680

Closed

nrmancuso force-pushed the issue-7190-remove-dep-setclassloader-methods branch 2 times, most recently from 44e9aac to 1034af4 Compare March 3, 2020 22:15

nrmancuso mentioned this pull request Mar 4, 2020

upgrade maven-checkstyle-plugin to 3.1.1 xwiki/xwiki-commons#88

Merged

nrmancuso mentioned this pull request Mar 5, 2020

config: upgrade maven-checkstyle-plugin to 3.1.1 sevntu-checkstyle/sevntu.checkstyle#804

Merged

nrmancuso force-pushed the issue-7190-remove-dep-setclassloader-methods branch from 1034af4 to 7dffd09 Compare March 5, 2020 18:29

nrmancuso mentioned this pull request Mar 5, 2020

upgrade maven-checkstyle-plugin to 3.1.1 apache/apex-core#609

Closed

nrmancuso mentioned this pull request Mar 11, 2020

minor: upgrade maven-checkstyle-plugin to 3.1.1 sevntu-checkstyle/methods-distance#52

Merged

nrmancuso mentioned this pull request Mar 11, 2020

minor: upgrade maven-checkstyle-plugin checkstyle/contribution#460

Merged

nrmancuso force-pushed the issue-7190-remove-dep-setclassloader-methods branch from 86e9c7c to 980a3b0 Compare March 11, 2020 11:42

romani approved these changes Mar 11, 2020

View reviewed changes

rnveach suggested changes Mar 11, 2020

View reviewed changes

nrmancuso force-pushed the issue-7190-remove-dep-setclassloader-methods branch 2 times, most recently from 1df00c6 to 9f3a670 Compare March 11, 2020 21:14

rnveach self-assigned this Mar 11, 2020

nrmancuso added 4 commits March 11, 2020 17:15

Issue checkstyle#7190: remove deprecated setClassLoader methods

b39f338

Issue checkstyle#7190: update Orekit and Hipparcus checkout commits

29d04a0

Issue checkstyle#7190: switch htmlunit to github

d441278

Issue checkstyle#7190: update xwiki SHA

9f3a670

rnveach suggested changes Mar 11, 2020

View reviewed changes

rnveach approved these changes Mar 12, 2020

View reviewed changes

rnveach merged commit 13f067c into checkstyle:master Mar 12, 2020

This was referenced Mar 16, 2020

minor: improve clean up of .ci-temp directory #7870

Closed

Improve clean up of .ci-temp directory #7892

Closed

Uh oh!

Conversation

nrmancuso commented Mar 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Mar 4, 2020

Uh oh!

rnveach commented Mar 4, 2020

Uh oh!

romani commented Mar 4, 2020

Uh oh!

rnveach commented Mar 4, 2020

Uh oh!

nrmancuso commented Mar 4, 2020

Uh oh!

nrmancuso commented Mar 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Mar 5, 2020

Uh oh!

romani commented Mar 5, 2020

Uh oh!

nrmancuso commented Mar 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Mar 5, 2020

Uh oh!

rnveach commented Mar 5, 2020

Uh oh!

nrmancuso commented Mar 6, 2020

Uh oh!

romani commented Mar 6, 2020

Uh oh!

rnveach commented Mar 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Mar 7, 2020

Uh oh!

nrmancuso commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 7, 2020

Uh oh!

romani commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Mar 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 7, 2020

Uh oh!

rnveach commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

romani commented Mar 11, 2020

Uh oh!

rnveach commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nrmancuso commented Mar 11, 2020

Uh oh!

nrmancuso commented Mar 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

nrmancuso commented Mar 3, 2020 •

edited

Loading

nrmancuso commented Mar 4, 2020 •

edited

Loading

nrmancuso commented Mar 5, 2020 •

edited

Loading

rnveach commented Mar 6, 2020 •

edited

Loading

nrmancuso commented Mar 7, 2020 •

edited

Loading

nrmancuso commented Mar 7, 2020 •

edited

Loading

rnveach commented Mar 7, 2020 •

edited

Loading

romani commented Mar 7, 2020 •

edited

Loading

nrmancuso commented Mar 7, 2020 •

edited

Loading

rnveach commented Mar 7, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

nrmancuso commented Mar 11, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

nrmancuso commented Mar 11, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

nrmancuso commented Mar 11, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

nrmancuso commented Mar 11, 2020 •

edited

Loading

rnveach commented Mar 11, 2020 •

edited

Loading

rnveach Mar 11, 2020 •

edited

Loading

rnveach Mar 11, 2020 •

edited

Loading