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

maven indexing: update lucene from 9.9.1 to 9.10.0. #7087

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

mbien
Copy link
Member

@mbien mbien commented Feb 20, 2024

(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.

@mbien mbien added Upgrade Library Library (Dependency) Upgrade Maven [ci] enable "build tools" tests labels Feb 20, 2024
@mbien mbien added this to the NB22 milestone Feb 20, 2024
Copy link
Member Author

@mbien mbien Feb 20, 2024

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
Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Member Author

@mbien mbien Feb 21, 2024

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

@mbien
Copy link
Member Author

mbien commented Feb 27, 2024

resolved conflict and refreshed PR

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.

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
Copy link
Contributor

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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)
@mbien
Copy link
Member Author

mbien commented Feb 28, 2024

post review license summary:

@mbien
Copy link
Member Author

mbien commented Feb 28, 2024

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.

@mbien mbien merged commit 447bc15 into apache:master Feb 28, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maven [ci] enable "build tools" tests Upgrade Library Library (Dependency) Upgrade
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants