-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
Hey @Zha0q1 , Thanks for submitting the PR
CI supported jobs: [centos-cpu, windows-cpu, website, miscellaneous, sanity, centos-gpu, unix-gpu, windows-gpu, edge, unix-cpu, clang] Note: |
@samskalicky @leezu Could you review thanks! |
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.
You're attaching a bunch of licenses under the keyword "OpenMP", but it's not clear to what files the apply. We need to specify the licenses with respect to the files (subfolders) they apply to.
Sometimes, multiple licenses may apply to a particular code and we should state if the users can choose one of the or what their relation is
Those licenses are from the master License.txt in the root directory. There they do not specify to which submodules those license apply to. In fact there is no clear submodule by the way they structure the directories https://github.com/llvm-mirror/openmp/tree/b76842ed16984ae5edcbbc4b00a94fda20419431 |
You need to read the license file. It states:
as well as
Based on these instructions, you need to modify the MXNet LICENSE so that it separately lists
The latter may best be scripted. |
The easiest option here is to delete the 3rdparty/openmp folder as it's unused by the default Makefile build and optional for the cmake build. |
I totally agree with this methodology but I checked all the file, and there was no mentioning of any license other than Apache 2.0. I tried to associate those extra licenses with specific files/submodules but I could not find any such links. |
In that case you don't need to include them. Referencing the LICENSE.txt is sufficient. We don't need to copy the license text into our LICENSE file. We only need to ensure that we point to each file / set of files of a particular license and provide a pointer where the user can read their license |
Sounds good. I have deleted these licenses from our file. Given our discussion I think it suffices to update the copyright statement year |
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.
3rdparty/openmp/LICENSE.txt specifies Apache License v2.0 with LLVM Exceptions not MIT License
@leezu Yeah sorry did not notice that. I moved it under the apache list and also attached the llvm exception at the bottom |
#19427