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

chore: fix StringEncoding license & add multi license check action #227

Merged
merged 7 commits into from
Feb 9, 2023

Conversation

simon824
Copy link
Member

@simon824 simon824 commented Jan 17, 2023

@codecov

This comment was marked as off-topic.

@@ -1,19 +1,3 @@
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

hi @WillemJiang can we keep both of the two licenses in a source file?

Copy link
Member

Choose a reason for hiding this comment

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

Did we make some big changes to the code?
If not,we cannot add copyright information to this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -1,19 +1,3 @@
/*
Copy link
Member

Choose a reason for hiding this comment

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

Did we make some big changes to the code?
If not,we cannot add copyright information to this file.

NOTICE Outdated
Apache Cassandra
Copyright 2022 The Apache Software Foundation

cassandra-hadoop-util/src/main/java/org/apache/cassandra/db/SystemKeyspace.java
Copy link
Member

Choose a reason for hiding this comment

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

We need to put this information into the License file instead of the NOTICE file.

Copy link
Member Author

Choose a reason for hiding this comment

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

These information in the NOTICE are copy from janusgraph NOTICE.txt.
It seems that the upstream NOTICE is not standardized, how should we deal with this situation?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to copy all these license-related information from janusgraph NOTICE file.

NOTICE Show resolved Hide resolved
@imbajin
Copy link
Member

imbajin commented Jan 18, 2023

Another question is:
Do we need to bring the reference information of the source code LICENSE in the root to dist/xx/LICENSE for the binary package?

Now we only include it in the source package's LICENSE

@WillemJiang
Copy link
Member

Another question is: Do we need to bring the reference information of the source code LICENSE in the dist/xx/LICENSE of the binary package?

Now we only include it in the source package's LICENSE

Yes, we need to include the source code 'LICENSE' information in the binary release 'LICENSE'.

@imbajin imbajin added the apache label Jan 19, 2023
@imbajin imbajin added this to the 1.0.0 milestone Jan 19, 2023
@imbajin imbajin changed the title chore: fix StringEncoding license chore: fix StringEncoding license & add multi license check action Feb 8, 2023
The following components are provided under the Apache License. See project link for details.
The text of each license is the standard Apache 2.0 license.

computer-core/src/main/java/org/apache/hugegraph/computer/core/util/StringEncoding.java files from https://github.com/JanusGraph
Copy link
Member

Choose a reason for hiding this comment

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

@javeme maybe this file should move to commons, so that we don't need to import the same refer in multi repo

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 it's ok

Comment on lines +502 to +505
Third party Go license
========================================================================

The following components are provided under The Go licens.
The following components are provided under The Go license.
Copy link
Member

Choose a reason for hiding this comment

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

@simon824 I correct the typo, ensure is this as expected?

@@ -44,7 +28,6 @@

/**
* @author Matthias Broecheler (me@matthiasb.com)
* @author HugeGraph Authors
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure whether we can add a new author, so temporarily remove it

Restore it after confirmation maybe

javeme
javeme previously approved these changes Feb 8, 2023
The following components are provided under the Apache License. See project link for details.
The text of each license is the standard Apache 2.0 license.

computer-core/src/main/java/org/apache/hugegraph/computer/core/util/StringEncoding.java files from https://github.com/JanusGraph
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 it's ok

javeme
javeme previously approved these changes Feb 8, 2023
@@ -63,7 +46,7 @@ public final class StringEncoding {
private static final Base64.Encoder BASE64_ENCODER = Base64.getEncoder();
private static final Base64.Decoder BASE64_DECODER = Base64.getDecoder();

// Similar to {@link StringSerializer}
/** Similar to {@link StringSerializer} */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* is ok?

Copy link
Member

Choose a reason for hiding this comment

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

/* is ok?

shouldn't use /* for class property, refer here

@imbajin imbajin merged commit b1f7346 into apache:master Feb 9, 2023
imbajin added a commit that referenced this pull request Feb 10, 2023
)

* add license check action & use proper "UTF-8" charset

* exclude generate go files

* include release-docs in binary release

* Update NOTICE

---------

Co-authored-by: imbajin <jin@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants