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

Update dependency commons-io:commons-io to v2.14.0 #1653

Merged
merged 1 commit into from
Oct 1, 2023

Conversation

solrbot
Copy link
Collaborator

@solrbot solrbot commented May 21, 2023

This PR contains the following updates:

Package Type Update Change
commons-io:commons-io (source) dependencies minor 2.11.0 -> 2.14.0

Configuration

πŸ“… Schedule: Branch creation - "* * * * 0" (UTC), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

β™» Rebasing: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

πŸ”• Ignore: Close this PR and you won't be reminded about this update again.


  • If you want to rebase/retry this PR, check this box

This PR has been generated by Renovate Bot

@solrbot solrbot force-pushed the renovate/commons-io-commons-io-2.x branch from 1dd8168 to 151d3da Compare May 24, 2023 11:48
@janhoy janhoy requested a review from uschindler June 7, 2023 10:48
@solrbot solrbot force-pushed the renovate/commons-io-commons-io-2.x branch from ac6dd9a to 2978ebe Compare June 7, 2023 14:26
Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

I don't see the forbidden signatures version fix.

// .findAll { id -> id.group == 'commons-io' && id.name == 'commons-io' }
// .collect { id -> "${id.name}-unsafe-${id.version}" as String }
// Hack until forbiddenapis supports commons-io 2.12.0
bundledSignatures += ['commons-io-unsafe-2.11.0']
Copy link
Contributor

Choose a reason for hiding this comment

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

This now adds commons-io to all Solr modules, although it may not be part of dependencies. This should not hurt, but could cause failures if we have a Solr module that does not refer to commons-io.

I'd change only line 56 to be:

.collect { id -> "${id.name}-unsafe-2.11.0" as String }

This ensures the forbiddenapis are only added if the compilation unit depends on commons-io.

Maybe keep the existing line commented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, I see commons-IO yesterday released 2.13.0 to fix some regressions in 2.12. In fact I think the test failure on this PR may be due to that.. I'll re-purpose this to upgrade to 2.13

Copy link
Contributor

Choose a reason for hiding this comment

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

Upgraded to 2.13.0 and made the workaround hack better..

Copy link
Contributor

Choose a reason for hiding this comment

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

I have seen your PR for commons-io. I will update this.

Theres also another forbiddenapis issue to make the version parsing a bit better for commons-io. So whenever a newer version is passed to the bundled signatures it will fallback to the last recent version before the requested one.

This should solve issues like this in future. I just have no time before bbuzz.... (my talk is still mostly unprepared)

@janhoy janhoy changed the title Update dependency commons-io:commons-io to v2.12.0 Update dependency commons-io:commons-io to v2.13.0 Jun 8, 2023
@uschindler
Copy link
Contributor

Somehow your solr bot tried to force-push his own changes over and over.

@janhoy
Copy link
Contributor

janhoy commented Jun 8, 2023

Somehow your solr bot tried to force-push his own changes over and over.

Yea, I think I made a mistake in my first push. Now it is blocked so that's better. But there are test failures that I believe relate to the upgrade, so will change this to draft and investigate.

@janhoy janhoy marked this pull request as draft June 8, 2023 10:39
@janhoy
Copy link
Contributor

janhoy commented Jun 8, 2023

Confirmed that TestCoreContainer.testDeleteBadCores fails reproducibly with v2.13.0, while it passes with v2.11.

The test attempt to clean up after a "bad core" where the instanceDir does not exist.

commons-io v2.11 throws java.nio.file.NoSuchFileException which is caught in https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/core/SolrCore.java#L3334

But in commons-io v2.13 it throws java.lang.IllegalArgumentException with message: "File system element for parameter 'directory' does not exist":

java.lang.IllegalArgumentException: File system element for parameter 'directory' does not exist: '/Users/janhoy/git/solr/solr/core/build/tmp/tests-tmp/solr.core.TestCoreContainer_FC7F3677582ED11B-001/tempDir-001/badcore/data'
	at __randomizedtesting.SeedInfo.seed([FC7F3677582ED11B:8F28B8ED8131FDE0]:0)
	at org.apache.commons.io.file.PathUtils.requireExists(PathUtils.java:1396)
	at org.apache.commons.io.file.PathUtils.visitFileTree(PathUtils.java:1653)
	at org.apache.commons.io.file.PathUtils.lambda$deleteDirectory$0(PathUtils.java:503)
	at org.apache.commons.io.file.PathUtils.withPosixFileAttributes(PathUtils.java:1774)
	at org.apache.commons.io.file.PathUtils.deleteDirectory(PathUtils.java:502)
	at org.apache.commons.io.file.PathUtils.deleteDirectory(PathUtils.java:487)
	at org.apache.solr.core.SolrCore.deleteUnloadedCore(SolrCore.java:3333)
	at org.apache.solr.core.CoreContainer.unload(CoreContainer.java:2083)
	at org.apache.solr.core.TestCoreContainer.testDeleteBadCores(TestCoreContainer.java:341)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:77)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:568)

Spent some time digging, but lots of code has changed since last release in these delete calls. Have no time to pursue yet..

@janhoy
Copy link
Contributor

janhoy commented Jun 8, 2023

I filed https://issues.apache.org/jira/browse/IO-800 for this, to see if this rings a bell upstream...

@janhoy
Copy link
Contributor

janhoy commented Jun 8, 2023

I filed https://issues.apache.org/jira/browse/IO-800 for this, to see if this rings a bell upstream...

The commons-io project merged my PR, so when 2.13.1 is released our failing test will once again succeed. We could have changed our try/catch but better to preserve back-compat. Leaving this in draft state for now.

@cpoerschke
Copy link
Contributor

Oops, I checked the "If you want to rebase/retry this PR, check this box" option before fully reading the PR history here ... unchecked it again now but not sure if @solrbot will see that in time.

@janhoy
Copy link
Contributor

janhoy commented Sep 11, 2023

Yea, looks like the bot foce pushed. Unfortunately the commons project have not released a 2.13.1 or 2.14, even if 2.13.0 contains a non-compatible change :( So we just have to wait until their next release I suppose, to get an upgrade that is compatible with what we have...

@solrbot solrbot changed the title Update dependency commons-io:commons-io to v2.13.0 Update dependency commons-io:commons-io to v2.14.0 Sep 30, 2023
@solrbot solrbot force-pushed the renovate/commons-io-commons-io-2.x branch from ac7b163 to 183d8b0 Compare September 30, 2023 12:33
@janhoy janhoy marked this pull request as ready for review September 30, 2023 13:23
@janhoy
Copy link
Contributor

janhoy commented Sep 30, 2023

I’m on holiday, please someone review and merge this. It will unblock other dependency upgrades too.

@uschindler
Copy link
Contributor

When #1971 is merged, the upgrade should succeed as 2.14.0 is supported by forbiddenapis.

@uschindler
Copy link
Contributor

I retriggered the jobs.

@uschindler
Copy link
Contributor

Oh I think the jobs are not running of the diff against main branch, i think the bot has to merge his branch first. No idea how to tell it to do this.

@solrbot solrbot force-pushed the renovate/commons-io-commons-io-2.x branch from 183d8b0 to 1775574 Compare October 1, 2023 16:24
@uschindler
Copy link
Contributor

Oh I think the jobs are not running of the diff against main branch, i think the bot has to merge his branch first. No idea how to tell it to do this.

There was a checkbox on top of PR.

Copy link
Contributor

@uschindler uschindler left a comment

Choose a reason for hiding this comment

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

Looks like this time upgrade worked well. No test failures anymore.
I'll going to merge this.

@uschindler uschindler merged commit 690aaac into apache:main Oct 1, 2023
2 checks passed
@uschindler uschindler self-assigned this Oct 1, 2023
@solrbot solrbot deleted the renovate/commons-io-commons-io-2.x branch October 1, 2023 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants