Jandex: reindex if the index is too new#47283
Conversation
|
|
||
| private static class MetaInfJandexReader implements Function<PathVisit, Index> { | ||
| private static MetaInfJandexReader instance; | ||
| private final String treeDescription; |
There was a problem hiding this comment.
I added this because visit.getRoot() often returns just / (e.g. in case of a JAR from local Maven repo), so that's quite useless. PathTree.toString() is much more descriptive (e.g. in case of a JAR from local Maven repo, it returns the full path).
|
I think we would like to backport this to 3.20, but I'll leave the decision up to you :-) |
gastaldi
left a comment
gastaldi
left a comment
There was a problem hiding this comment.
LGTM, +1 to backport this to 3.20
This comment has been minimized.
This comment has been minimized.
|
The tests are failing with: I think we have a couple of actions here:
|
The messages include the parameters, because interpolation happens further down the line.
I've got a commit locally that does just that. Testing now if I've covered all tests.
I think these PRs should fix this:
I'll check the log if there's more, just to be sure. |
|
I see 4 messages, two are coming from the same project? |
|
These are the messages I see in one randomly selected test: These are all from the 3 projects I linked above. |
|
Perhaps we can remove the |
|
Good point @gastaldi, will do! |
| } | ||
| return reader.read(); | ||
| } catch (UnsupportedVersion e) { | ||
| log.warnf("Reindexing %s, the index is too new (%s)", file, e.getMessage()); |
There was a problem hiding this comment.
I'm not that excited about the message. Maybe ..., the index format of %s is too new for the Jandex version used by your application. Please report it to the author of this jar..
Or similar. The problem is about the index format, not the index and we should really avoid cryptic messages. And also, we should be clear that it's something to fix at the library level, not in your app.
The last part might be something to include in the previous message too.
Feel free to improve the wording, it's just to give a general direction.
Obviously, if we adjust things here, we need to do the same below.
There was a problem hiding this comment.
I'll try to improve this.
We've been bitten by this issue many times in the past: if a dependency upgrades to a newer Jandex than what Quarkus contains, build fails with an `UnsupportedVersion` exception. This commit is a proper solution: if the exception is thrown, we swallow it and reindex the archive with the Jandex version that Quarkus incorporates. Further, this commit increments the minimum Jandex version we "trust" from Jandex 2.1 to Jandex 3.0. We rely on new features from Jandex 2.4 and 3.0 on many places, so it makes sense to make 3.0 the new minimum. The changes added in 3.2 and 3.3 are also important, but are not used heavily yet.
Ladicek
force-pushed
the
jandex-auto-reindexing
branch
from
649f8c4 to
97572fa
Compare
gsmet
added
the
triage/waiting-for-ci
|
@Ladicek it would be nice if we could have releases for the jars mentioned above before we push 3.22 so that we avoid reindexing every time we build an application. |
|
That should be doable. |
Status for workflow
|
quarkus-bot
Bot
removed
the
triage/waiting-for-ci
4 participants
We've been bitten by this issue many times in the past: if a dependency upgrades to a newer Jandex than what Quarkus contains, build fails with an
UnsupportedVersionexception.This commit is a proper solution: if the exception is thrown, we swallow it and reindex the archive with the Jandex version that Quarkus incorporates.
Further, this commit increments the minimum Jandex version we "trust" from Jandex 2.1 to Jandex 3.0. We rely on new features from Jandex 2.4 and 3.0 on many places, so it makes sense to make 3.0 the new minimum. The changes added in 3.2 and 3.3 are also important, but are not used heavily yet.