-
Notifications
You must be signed in to change notification settings - Fork 874
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
maven indexing: update lucene from 9.9.1 to 9.10.0. #7087
Conversation
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 copied the file from the the other lib wrappers which use annotaiton-api-1.3.2.
btw I am not sure why the jar is needed since maven embedder exports this jar too, but it doesn't work without it.
Origin: Apache Software Foundation | ||
License: Apache-2.0 | ||
URL: https://lucene.apache.org/ | ||
Source: https://github.com/apache/lucene | ||
Files: lucene-analysis-common-9.9.1.jar lucene-backward-codecs-9.9.1.jar lucene-core-9.9.1.jar lucene-highlighter-9.9.1.jar lucene-queryparser-9.9.1.jar | ||
Files: lucene-analysis-common-9.10.0.jar lucene-backward-codecs-9.10.0.jar lucene-core-9.10.0.jar lucene-highlighter-9.10.0.jar lucene-queryparser-9.10.0.jar | ||
|
||
Apache License |
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.
So, while here, it would probably be good to fix the license text. If I look e.g. inside the lucene-analysis-common-9.10.0.jar
, I see META-INF/LICENSE.txt
, which contains a lot more text than what's here (as here is Apache 2.0 only).
As a side note, the license text in lucene-analysis-common-9.10.0.jar/META-INF/LICENSE.txt
is not completely correct, to my knowledge: the NOTICE.txt says this:
The German,Spanish,Finnish,French,Hungarian,Italian,Portuguese,Russian and Swedish light stemmers
(common) are based on BSD-licensed reference implementations created by Jacques Savoy and
Ljiljana Dolamic. These files reside in:
at least some of the files listed below this note seem to be included in the jar (in compiled form), but there's no license text for this in LICENSE.txt. Based on history, I assume we want be fixing those, but its worth noting, I think.
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.
Agree with @lahodaj - sounds like reference to BSD should be in LICENSE.
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 can try to improve this. So far I copied the license of lucene-analysis-common
into our license file which includes more sections and also the BSD section. I am not quite sure what else i should do.
Some license files were badly formatted
java/maven.indexer/external/lucene-9.10.0-license.txt has a trailing space on line #221
Some license files have incorrect headers
java/maven.indexer/external/lucene-9.10.0-license.txt contains a license body which does not match that in nbbuild/licenses/Apache-2.0: unexpected extra content
going to have to fix this too now I suppose and setup a new license.
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.
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 think having the license match what's in the jars in acceptable for now. We need to file a bug against Lucene to fix their license at some point.
resolved conflict and refreshed PR |
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.
Short test indicates that indexing works after the update and did not show suprising runtimes. I left an inline comment about the javax.annotation-api
license, but that is it.
@@ -0,0 +1,405 @@ | |||
Name: Javax Annotation API | |||
Version: 1.3.2 | |||
License: CDDL-1.1 |
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.
According to the META-INF/LICENSE.txt
file the whole JAR is CDDL 1.0 or GPL-v2-CPE. I suggest to mark it as such CDDL-GPL-2-CP
(the license text in the licenses folder is a readable version).
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, copied the license from the license folder into this file https://github.com/apache/netbeans/compare/eb31fc81fdbc0432d596605828a2fcf149d96e6b..e2a8ab94ca9ef4d4641f3914cc2ea053986ceaa0
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.
Not convinced about that. If we have a choice of license we should pick the one we're using, which in this case is CDDL.
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.
btw I originally simply copied it over from other lib wrappers #7087 (comment)
quick search:
https://github.com/search?q=repo%3Aapache%2Fnetbeans+javax.annotation-api-1.+language%3AText&type=code&l=Text
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 should learn and just ignore license problems. To be frank I don't care. Lets keep it consistent and revert to the initial state of CDDL-1.1. Sorry for the noise.
- maintenance release - can now use (final) foreign memory API on JDK 22 - can now use (preview) vector API on JDK 22 too (note: the APIs were already active on 21 before)
post review license summary:
|
going to merge this since this is essentially reviewed I think. Matthias and me tested it too. Thanks with helping with the licenses everyone - even if this isn't perfect yet, at least it is a little better than before. |
(note: lucene used the two mentioned APIs on JDK 21 already, but it couldn't use them on JDK 22 yet)
tested full remote/local indexing and SMO queries.