add com.janeluo.ikkanalyzer dependency to core model #2206

KeeProMise · 2023-05-08T08:51:44Z

When I run the code in the example to learn the database, I find that the class org/wltea/analyzer/core/IKSegmenter cannot be found. The scope of this class in the module core is provided, and the module example depends on the module core.

imbajin · 2023-05-08T09:20:06Z

Yep, because it's under the GPL license which is not compatible with Apache License 2.0, so we have to set the scope to provided and let users to download & config it by themselves 😿

But we do lack the doc to tell the users (so as the mysql dependency - refer #2175 (comment)) , could add some doc & comments in the config template together

And maybe the example should also remove the dependency? @zyxxoo @javeme (also we should enhance the doc for example for new users/devs to start

codecov · 2023-05-08T09:24:53Z

Codecov Report

Merging #2206 (b84308b) into master (267ff6d) will increase coverage by 3.51%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##             master    #2206      +/-   ##
============================================
+ Coverage     61.53%   65.04%   +3.51%     
- Complexity      484      979     +495     
============================================
  Files           497      498       +1     
  Lines         40572    40681     +109     
  Branches       5663     5681      +18     
============================================
+ Hits          24965    26461    +1496     
+ Misses        13061    11595    -1466     
- Partials       2546     2625      +79

see 31 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

KeeProMise · 2023-05-08T09:27:19Z

Yep, because it's under the GPL license which is not compatible with Apache License 2.0, so we have to set the scope to provided and let users to download & config it by themselves 😿

But we do lack the doc to tell the users (so as the mysql dependency - refer #2175 (comment)) , could add some doc & comments in the config template together

And maybe the example should also remove the dependency? @zyxxoo @javeme (also we should enhance the doc for example for new users/devs to start

Is it possible to provide a readme in the module example, explaining the dependencies

imbajin · 2023-05-08T09:40:14Z

Is it possible to provide a readme in the module example, explaining the dependencies

Yep, should add the doc/comments in these spaces:

add the details in the doc repo to tell users how to config the dependence (like ikka/mysql)
refer a doc link in the README.md for users to know this ⚠️
add the comment in config template to remind users to view doc link configuration (could also add the comment in the relative code 👨🏻‍💻)

liuxiaocs7 · 2023-05-30T12:39:39Z

Same question, hi, @imbajin

Yep, because it's under the GPL license which is not compatible with Apache License 2.0, so we have to set the scope to provided and let users to download & config it by themselves 😿

The ik tokenizer we used seems to be this one: https://mvnrepository.com/artifact/com.janeluo/ikanalyzer/2012_u6 rather than https://github.com/blueshen/ik-analyzer

incubator-hugegraph/hugegraph-core/pom.xml

Lines 198 to 203 in f23c648

    
           <dependency> 
        
               <groupId>com.janeluo</groupId> 
        
               <artifactId>ikanalyzer</artifactId> 
        
               <version>${ikanalyzer.version}</version> 
        
               <scope>provided</scope> 
        
           </dependency>

The license from mvn website, does it meet the requirements?

Also this one works fine by installing in local repository.

imbajin · 2023-05-30T13:19:42Z

Same question, hi, @imbajin

Yep, because it's under the GPL license which is not compatible with Apache License 2.0, so we have to set the scope to provided and let users to download & config it by themselves 😿

The ik tokenizer we used seems to be this one: mvnrepository.com/artifact/com.janeluo/ikanalyzer/2012_u6 rather than blueshen/ik-analyzer

incubator-hugegraph/hugegraph-core/pom.xml

Lines 198 to 203 in f23c648

<dependency>

<groupId>com.janeluo</groupId>

<artifactId>ikanalyzer</artifactId>

<version>${ikanalyzer.version}</version>

<scope>provided</scope>

</dependency>

The license from mvn website, does it meet the requirements?

Also this one works fine by installing in local repository.

Thanks for the reminder, I thinks it's fine to use it (but not sure)

@javeme @zyxxoo Is it possible to adopt other implements? Consider replacing it

javeme · 2023-05-30T14:31:10Z

I think the ikanalyzer license is Apache compatible, but not sure the reason we removed ikanalyzer from #2100

zyxxoo · 2023-05-31T06:14:02Z

OK, It seems that we can now use this dependency. @KeeProMise @liuxiaocs7

KeeProMise · 2023-06-02T06:29:36Z

OK, It seems that we can now use this dependency. @KeeProMise @liuxiaocs7

I think that since ikanalyzer can be used in the core module, it can also be used in the example module.

imbajin · 2023-06-02T07:14:00Z

OK, It seems that we can now use this dependency. @KeeProMise @liuxiaocs7

I think that since ikanalyzer can be used in the core module, it can also be used in the example module.

yep, just fix the dependence check (refer here, then we could merge it back)

And we could also add the dependence doc in the PR template or Wiki? (for others to know it)

KeeProMise · 2023-06-02T15:10:13Z

com.janeluo.ikkanalyze、lucene-queries-4.7.2.jar、lucene-queryparser-4.7.2.jar 、lucene-sandbox-4.7.2.jar is Apache 2.0, so I added the above-mentioned third-party dependencies in the known-dependencies.txt file, and added com.janeluo.ikkanalyze to the core module

imbajin

Thanks for the contribution, and if we add a new dependency in project, we need also update the info in LICENSE & NOTICE file both in the root & dist modules (depend on source or binary dependency)

@simon824 remove provided option, shall we add/update info in dist (LICENSE & NOTICE) file (seems yes?)
And we need a doc to help new devs know how to add/update the info

simon824 · 2023-06-06T06:49:18Z

Hi @KeeProMise ,
If we want to add a new third-party dependency we need to do the following things:

Find the third-party dependency repository, and put the dependency license file in [./hugegraph-dist/release-docs/licenses/](https://github.com/apache/incubator-hugegraph/tree/master/hugegraph -dist/release-docs/licenses) path.
Declare the dependency in ./hugegraph-dist/release-docs/LICENSE LICENSE information.
Find the NOTICE file in the repository and append it to [./hugegraph-dist/release-docs/NOTICE](https://github.com/apache/incubator-hugegraph/blob/master/hugegraph-dist/release-docs /NOTICE) (skip this step if there is no NOTICE file).
Execute script ./hugegraph-dist/scripts/dependency/regenerate_known_dependencies.sh to update dependency list known-dependencies.txt (or manually update).

Update: refer with example

KeeProMise · 2023-06-06T09:09:50Z

Hi @KeeProMise , If we want to add a new third-party dependency we need to do the following things:

Find the third-party dependency repository, and put the dependency license file in [./hugegraph-dist/release-docs/licenses/](https://github.com/apache/incubator-hugegraph/tree/master/hugegraph -dist/release-docs/licenses) path.

Declare the dependency in ./hugegraph-dist/release-docs/LICENSE LICENSE information.

Find the NOTICE file in the repository and append it to [./hugegraph-dist/release-docs/NOTICE](https://github.com/apache/incubator-hugegraph/blob/master/hugegraph-dist/release-docs /NOTICE) (skip this step if there is no NOTICE file).

Execute script ./hugegraph-dist/scripts/dependency/regenerate_known_dependencies.sh to update dependency list known-dependencies.txt (or manually update).

Thank you for your guidance. I'll do it

…y in LICENSE info

KeeProMise · 2023-06-07T01:58:37Z

The third-party dependency does not have a NOTICE file, so I took these three steps:

Find the third party dependency repository, and put the dependency license file in ./huggegraph-dist/release docs/licenses/ path
Declare the dependency in /Hugegraph-dist/release-docs/License License information
Execute script /Hugegraph-dist/scripts/dependency/regenerate_ Known_ Dependencies. sh to update dependency list knowledge dependencies. txt

imbajin · 2023-06-07T11:43:44Z

hugegraph-dist/release-docs/licenses/LICENSE-ikanalyzer-2012_u6.txt

@simon824 can we ignore the standard Apache 2.0 license file? (seems they are duplicate?)

Not sure, I'll check it

javeme

LGTM

imbajin

@simon824 address it later https://github.com/apache/incubator-hugegraph/pull/2206/files#r1221441232

merge this PR first

KeePromMise added 2 commits May 8, 2023 16:46

add com.janeluo.ikkanalyzer dependency to example model

fc7950f

add com.janeluo.ikkanalyzer dependency to example model

2622ab7

imbajin added dependencies Incompatible dependencies of package doc Document expected apache labels May 8, 2023

KeePromMise added 2 commits June 2, 2023 22:35

add com.janeluo.ikkanalyzer dependency to example model

ed370af

add com.janeluo.ikkanalyzer dependency to core model

2773e6c

KeeProMise changed the title ~~add com.janeluo.ikkanalyzer dependency to example model~~ add com.janeluo.ikkanalyzer dependency to core model Jun 2, 2023

imbajin reviewed Jun 3, 2023

View reviewed changes

imbajin requested a review from simon824 June 3, 2023 09:49

put the third-party dependency license file and Declare the dependenc…

b84308b

…y in LICENSE info

KeeProMise requested a review from imbajin June 7, 2023 07:13

imbajin reviewed Jun 7, 2023

View reviewed changes

javeme approved these changes Jun 12, 2023

View reviewed changes

imbajin approved these changes Jun 20, 2023

View reviewed changes

imbajin merged commit 258d181 into apache:master Jun 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add com.janeluo.ikkanalyzer dependency to core model #2206

add com.janeluo.ikkanalyzer dependency to core model #2206

KeeProMise commented May 8, 2023

imbajin commented May 8, 2023 •

edited

Loading

codecov bot commented May 8, 2023 •

edited

Loading

KeeProMise commented May 8, 2023

imbajin commented May 8, 2023 •

edited

Loading

liuxiaocs7 commented May 30, 2023 •

edited

Loading

imbajin commented May 30, 2023 •

edited

Loading

javeme commented May 30, 2023

zyxxoo commented May 31, 2023 •

edited by imbajin

Loading

KeeProMise commented Jun 2, 2023

imbajin commented Jun 2, 2023

KeeProMise commented Jun 2, 2023

imbajin left a comment •

edited

Loading

simon824 commented Jun 6, 2023 •

edited by imbajin

Loading

KeeProMise commented Jun 6, 2023

KeeProMise commented Jun 7, 2023

imbajin Jun 7, 2023 •

edited

Loading

simon824 Jun 21, 2023

javeme left a comment

imbajin left a comment

add com.janeluo.ikkanalyzer dependency to core model #2206

add com.janeluo.ikkanalyzer dependency to core model #2206

Conversation

KeeProMise commented May 8, 2023

imbajin commented May 8, 2023 • edited Loading

codecov bot commented May 8, 2023 • edited Loading

Codecov Report

KeeProMise commented May 8, 2023

imbajin commented May 8, 2023 • edited Loading

liuxiaocs7 commented May 30, 2023 • edited Loading

imbajin commented May 30, 2023 • edited Loading

javeme commented May 30, 2023

zyxxoo commented May 31, 2023 • edited by imbajin Loading

KeeProMise commented Jun 2, 2023

imbajin commented Jun 2, 2023

KeeProMise commented Jun 2, 2023

imbajin left a comment • edited Loading

Choose a reason for hiding this comment

simon824 commented Jun 6, 2023 • edited by imbajin Loading

KeeProMise commented Jun 6, 2023

KeeProMise commented Jun 7, 2023

imbajin Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

simon824 Jun 21, 2023

Choose a reason for hiding this comment

javeme left a comment

Choose a reason for hiding this comment

imbajin left a comment

Choose a reason for hiding this comment

imbajin commented May 8, 2023 •

edited

Loading

codecov bot commented May 8, 2023 •

edited

Loading

imbajin commented May 8, 2023 •

edited

Loading

liuxiaocs7 commented May 30, 2023 •

edited

Loading

imbajin commented May 30, 2023 •

edited

Loading

zyxxoo commented May 31, 2023 •

edited by imbajin

Loading

imbajin left a comment •

edited

Loading

simon824 commented Jun 6, 2023 •

edited by imbajin

Loading

imbajin Jun 7, 2023 •

edited

Loading