- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3k
Jandex: reindex if the index is too new #47283
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
Conversation
|  | ||
| private static class MetaInfJandexReader implements Function<PathVisit, Index> { | ||
| private static MetaInfJandexReader instance; | ||
| private final String treeDescription; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-) | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try to improve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
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.
649f8c4    to
    97572fa      
    Compare
  
    | @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  | 
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.