[python-package] remove unused Eigen files, compile with EIGEN_MPL2_ONLY (fixes #3684) #3685
Conversation
jameslamb
requested review from
chivee,
henry0312,
huanzhang12,
Laurae2 and
wxchan
as code owners
| recursive-include compile/eigen/Eigen/src/misc * | ||
| recursive-include compile/eigen/Eigen/src/plugins * | ||
| recursive-include compile/eigen/Eigen/src/QR * | ||
| recursive-include compile/eigen/Eigen/src/SVD * |
There was a problem hiding this comment.
LightGBM only uses Eigen/Dense, and these other pieces are all the recursive dependencies of that.
| @@ -93,6 +93,9 @@ endif(USE_SWIG) | |||
| SET(EIGEN_DIR "${PROJECT_SOURCE_DIR}/eigen") | |||
| include_directories(${EIGEN_DIR}) | |||
|
|
|||
| # See https://gitlab.com/libeigen/eigen/-/blob/master/COPYING.README | |||
| ADD_DEFINITIONS(-DEIGEN_MPL2_ONLY) | |||
There was a problem hiding this comment.
Please add the same eigen define for VS solution file. For example,
LightGBM/windows/LightGBM.vcxproj
Line 104 in e95468c
There was a problem hiding this comment.
ok I tried in cc06c20, but I'm not that familiar with this file so please let me know if it's wrong
There was a problem hiding this comment.
Me neither... 😞
But I believe we can use global property for PreprocessorDefinitions and then in particular configurations append to global value.
https://docs.microsoft.com/en-us/cpp/build/working-with-project-properties?view=msvc-160#predefined-macros
LightGBM/windows/LightGBM.vcxproj
Line 172 in e95468c
I will try it tomorrow and let you know about results.
There was a problem hiding this comment.
@jameslamb OK, done in c05f391.
There was a problem hiding this comment.
thanks!
What is the effect of the changes in windows/LightGBM.vcxproj.filters from that commit? Should those changes have been made in #3299 ? And if so, is there some way we can catch that in CI?
There was a problem hiding this comment.
Should those changes have been made in #3299 ?
Yes.
And if so, is there some way we can catch that in CI?
No any I'm aware of.
I believe this is more aesthetic fix. I noticed this bug when opened solution file and saw these 2 new files listed outside of the project sources tree. However, seems that master is OK according to the following log lines at Appveyor:
running install
INFO:LightGBM:Starting to compile the library.
INFO:LightGBM:Starting to compile with MSBuild from existing solution file.
running build
There was a problem hiding this comment.
hmmm ok.
It seems that maybe this is just a file for configuring an editor and doesn't actually affect how the library is compiled:
- https://docs.microsoft.com/en-us/cpp/build/reference/vcxproj-filters-files?view=msvc-160
- https://stackoverflow.com/a/2841976/3986677
So I guess it makes sense that the build still succeeded without this change. I think maybe we could do something in CI that lists all files in src/ and checks that they show up in windows/vcxproj.filters, but I'd consider that low-priority now that I know it's just a config for an editor.
There was a problem hiding this comment.
@jameslamb Thanks for the info! I didn't know that
... the idea was that we did not want to trigger a rebuild simply because the filters changed, as they don't affect the build.
|
Since this PR will include the MPL2 preprocessor directive in the makefile and the VS project file, I think we can remove the following: |
oh thanks, sorry I didn't notice that before! I agree, that could be removed. |
|
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
This PR closes #3684. It makes the
MANIFEST.inrules related to the Eigen submode in more specific, to reduce the size of source distributions, such as the Python package produced bycd python-package && python setup.py sdist.masterThe wheel size (1.2M with gcc-10 on Mac 10.14) is unchanged by this PR.
While working through this, I realized there's a second purpose for being more specific: some pieces of Eigen are LGPL-licensed. LightGBM does not use those pieces and should not redistribute them.
Eigen is primarily MPL 2.0-licensed, which is compatible with LightGBM's (MIT-licensed) use of it. Eigen comes with a define,
EIGEN_MPL2_ONLY, which can be used to raise compilation errors if you accidentally include any Eigen code that is not MPL2-licensed or available under and even more permissive license like BSD. This PR proposes adding it.What About the R Package?
The R package does not use anything from Eigen right now, since the linear trees feature added in #3299 did not include the R package (documented in #3319). So no Eigen files are currently included in the CRAN package. Similar changes to this PR should be made when #3319 is implemented.