-
Notifications
You must be signed in to change notification settings - Fork 151
MADLIB-1076. Review LICENSE file and README.md #123
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
Conversation
|
Can one of the admins verify this patch? |
|
Looks good, thanks for the PR. One double check: the LICENSE file you are proposing refers explicitly to libstemmer, useLatex and pyyaml, but does not call out other 3rd party components by name. Is that your intention? |
8badbbc to
588244b
Compare
| CACHE STRING | ||
| "Base URL for Bitbucket projects. May be overridden for testing purposes.") | ||
| set(GITHUB_MADLIB_BASE_URL | ||
| set(EIGEN_BASE_URL |
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.
Since this is Eigen specific link, please include the eigen/archive part from line 55 in the URL itself. That way it's used specifically for that purpose.
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.
Will do. Thanks!
| @@ -1,10 +0,0 @@ | |||
| Portions of this software Copyright (c) 2010-2013 by EMC Corporation. All rights reserved. | |||
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.
Is this file necessary?
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.
This file is actually being replaced with a symlink to ../LICENSE -- it seems to be necessary for Mac OS X packaging. I can also replace it with a copy of LICENSE, but that'll be a tad messy.
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.
Thanks, Roman.
Symlink would be the best option if we have to keep the file.
Alternatively, we can change the "${CMAKE_SOURCE_DIR}/licenses/MADlib.txt" in deploy/PackageMaker/CMakeLists.txt to "${CMAKE_SOURCE_DIR}/LICENSE" and remove this file.
|
Jenkins, OK to test. |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
Refer to this link for build results (access rights to CI server needed): |
|
In file ReadMe_Build.txt the lines 38-40 should be updated from
to
|
No description provided.