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

Upgrade commons libs, asm, jgit, jna, jackson, felix, jacoco and junit #7237

Merged
merged 13 commits into from
Apr 8, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Apr 3, 2024

tried to find some low hanging fruits using the dependency checker: https://github.com/apache/netbeans/actions/runs/8543693772

one commit per library, if I should drop a commit please say so.

  • Upgrade jsch from 0.1.72 to 0.2.17 outside of jgit*s declared range
  • Upgrade jackson from 2.16.1 to 2.17.0
  • Upgrade jacoco.core from 0.8.7 to 0.8.12
  • Upgrade junit from 5.6.0 to 5.10.2
  • Upgrade JNA from 5.12.1 to 5.14.0
  • Upgrade felix from 7.0.3 to 7.0.5
  • Upgrade jgit from 9.8.0 to 9.9.0
    • Upgrade JavaEWAH from 1.1.6 to 1.2.3
  • Upgrade ASM from 9.6 to 9.7
  • Upgrade commons-compress from 1.25.0 to 1.26.1
  • Upgrade commons-logging from 1.3.0 to 1.3.1
  • Upgrade commons-io from 2.15.1 to 2.16.0
  • Upgrade commons-codec from 1.16.0 to 1.16.1

@mbien mbien added Upgrade Library Library (Dependency) Upgrade ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Apr 3, 2024
@mbien mbien added this to the NB22 milestone Apr 3, 2024
@mbien mbien removed the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Apr 3, 2024
@mbien mbien added the ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) label Apr 4, 2024
@ebarboni
Copy link
Contributor

ebarboni commented Apr 4, 2024

Nice.
What is the issue with jsch ?

@mbien
Copy link
Member Author

mbien commented Apr 4, 2024

Nice. What is the issue with jsch ?

I should have checked what version jgit needs first before updating that. Jgit is loaded as osgi module and it won't load dependencies which are outside the declared range -> classloader error. The manifest has com.jcraft.jsch;version="[0.1.37,0.2.0)" in it, so I updated out of range by accident.

mvnd eu.maveniverse.maven.plugins:toolbox:gav-tree -Dgav=org.eclipse.jgit:org.eclipse.jgit.ssh.jsch:6.9.0.202403050737-r
[INFO] --------------------------------[ pom ]---------------------------------
[INFO] 
[INFO] --- toolbox:0.1.3:gav-tree (default-cli) @ roller-project ---
[INFO] org.eclipse.jgit:org.eclipse.jgit.ssh.jsch:jar:6.9.0.202403050737-r
[INFO] +- org.eclipse.jgit:org.eclipse.jgit:jar:6.9.0.202403050737-r [compile]
[INFO] |  +- com.googlecode.javaewah:JavaEWAH:jar:1.2.3 [compile]
[INFO] |  \- commons-codec:commons-codec:jar:1.16.0 [compile]
[INFO] +- com.jcraft:jsch:jar:0.1.55 [compile]
[INFO] +- com.jcraft:jzlib:jar:1.1.3 [compile]
[INFO] \- org.slf4j:slf4j-api:jar:1.7.36 [compile]
[INFO] ------------------------------------------------------------------------

speaking of which... I should probably update JavaEWAH too.

edit: have to investigate that failing php test on windows

@mbien
Copy link
Member Author

mbien commented Apr 5, 2024

PHP test failure on win is likely file path length / max cp length / max arg size related. The forked JVM can't start. Will try to debug this on windows and check what is causing this.

alternative is to drop the commons-io commit since this seems to be sufficient to "resolve" this.

edit: suspicion confirmed, it is not the file path length itself, it is the gigantic classpath combined with the path prefix which goes over the process arg length limit. I could still make it test if I moved the workspace closer to the drive letter, going to try to replicate it in gh actions.

@mbien mbien force-pushed the lib-upgrades-nb22 branch 4 times, most recently from 266d869 to a016a69 Compare April 6, 2024 21:51
mbien added 13 commits April 7, 2024 05:23
 - both 'methodTag' and 'methodTest' seem to apply, CslTestBase L3150
   returns the first result which differs in some environments
 - moving cursor to the right avoids this issue
 - win has a 32k char process arg limit
 - test classpath args for the junit JVM can go over this limit if
   the workspace path is too long and the process will fail to start
 - this moves the workspace close to the drive letter as workaround
 - commons-io dependency added due to sig tests
and upgrade JavaEWAH from 1.1.6 to 1.2.3
@mbien mbien changed the title Upgrade commons libs, asm, jgit, jna, jackson, jsch, felix, jacoco and junit Upgrade commons libs, asm, jgit, jna, jackson, felix, jacoco and junit Apr 7, 2024
@mbien mbien marked this pull request as ready for review April 7, 2024 04:01
@mbien mbien added the CI continuous integration changes label Apr 7, 2024
Copy link
Contributor

@matthiasblaesing matthiasblaesing left a comment

Choose a reason for hiding this comment

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

Looks sane to me. Only eyeballed this.

checkCompletionDocumentation("testfiles/completion/documentation/php82/dnfTypes.php", "$this->methodT^ag($param1, $param2);", false, "");
checkCompletionDocumentation("testfiles/completion/documentation/php82/dnfTypes.php", "$this->methodTa^g($param1, $param2);", false, "");
Copy link
Member Author

@mbien mbien Apr 8, 2024

Choose a reason for hiding this comment

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

the interesting aspect about this issue here was that the test support classes take the first completion item and simply go with it (in this case 'methodTag' and 'methodTest'). It seems like what is first can depend on the environment which can cause test failures. For some reason the lib upgrade here guaranteed test failure on windows in this particular php test case.

CompletionProposal match = null;
for (CompletionProposal proposal : proposals) {
if (proposal.getName().startsWith(itemPrefix)) {
match = proposal;
break;
}
}

I think we should consider to fail the test right away when this loop returns more than one match since this would indicate that the test case is likely not stable and then update the tests. Or make sure that proposals has a stable order.

@mbien
Copy link
Member Author

mbien commented Apr 8, 2024

did some manual testing and everything worked fine, all tests green -> merging

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) CI continuous integration changes Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants