cmake: use object library to avoid duplicate compilation.

[cyfdecyf]

By using CMake's object libraries feature, we can avoid recompiling same source file multiple times.

This feature is introduced since CMake 2.8.8 and minmum required CMake version is 3.0 for LightGBM.

Seems like there's no separate compilation flags for building executable and library, thus I think it's safe to use object library to builld LightGBM.

References:

• Object Library on CMake wiki
• Object Library document

BTW, I guess following build code need some cleanup:

LightGBM/CMakeLists.txt

Lines 304 to 321 in 2370961

	if(MSVC)
	SET(variables
	CMAKE_C_FLAGS_DEBUG
	CMAKE_C_FLAGS_MINSIZEREL
	CMAKE_C_FLAGS_RELEASE
	CMAKE_C_FLAGS_RELWITHDEBINFO
	CMAKE_CXX_FLAGS_DEBUG
	CMAKE_CXX_FLAGS_MINSIZEREL
	CMAKE_CXX_FLAGS_RELEASE
	CMAKE_CXX_FLAGS_RELWITHDEBINFO
	)
	SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4 /MP")
	if(USE_DEBUG)
	SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /Od")
	else()
	SET(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /O2 /Ob2 /Oi /Ot /Oy")
	endif()
	else()

variables is not used in other places.

[cyfdecyf]

I have no clue for why https://github.com/microsoft/LightGBM/pull/4489/checks?check_run_id=3167177587 and https://github.com/microsoft/LightGBM/pull/4489/checks?check_run_id=3167177567 failed.

The same test using gcc have no problems.

[cyfdecyf]

Together with the failure of this test https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=10602&view=logs&j=5ea309bb-dea2-5bcb-538b-5c76ecf159eb, seems like there are some compiler flags (-fPIC) not being passed. But from CMakeLists.txt, -fPIC should be added when USE_DEBUG is not set.

[StrikerRUS]

Linking #4125.

[cyfdecyf]

@StrikerRUS Just to mention that this PR requires all target uses the same compilation options.

[StrikerRUS]

@cyfdecyf Sorry, could you please elaborate?

[cyfdecyf]

Sorry my previous statement is too vague.

Object libraries are targets in CMake and we can define compile options for them. When linking to object library in targets, we need to make sure the compile options of related targets are "compatible". For example, if one object library compiles without -fPIC, link with other files to generate shared library may result failure.

From my experiment, compile options does not propagate to depending targets which is reasonable behavior.

[cyfdecyf]

I tried to reproduce the compiling problem on my own macOS system. As I don't want to mess my system, I created a separate conda environement and installed the required R packages as in test_r_package.sh.

I'm not able to reproduce the same problem with the following bulid commands:

Rscript build_r.R --skip-install
R CMD check lightgbm_3.2.1.99.tar.gz --as-cran --run-donttest check_succeeded="no"

The compiler I'm using is Xcode command line tools with AppleClang 12.0.5.12050022. This is a little different than the CI environment.

So I'm changing build code to get more build output to investigate the issue. Sorry for the noise.

[cyfdecyf]

For CI of R-package on macOS using clang,

1. Compilation command before this PR:

/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DEIGEN_MPL2_ONLY -DLGB_R_BUILD -DMM_MALLOC -DMM_PREFETCH -DUSE_SOCKET -D_lightgbm_EXPORTS -I/Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/external_libs/eigen -I/Library/Frameworks/R.framework/Versions/3.6/Resources/include -I/Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/include -isystem /usr/local/include -Xclang -fopenmp -std=c++11 -pthread -Wextra -Wall -Wno-ignored-attributes -Wno-unknown-pragmas -Wno-return-type -O3 -fPIC -funroll-loops -isysroot /Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=10.15 -fPIC -F/Library/Frameworks  -Xclang -fopenmp -MD -MT CMakeFiles/_lightgbm.dir/src/c_api.cpp.o -MF CMakeFiles/_lightgbm.dir/src/c_api.cpp.o.d -o CMakeFiles/_lightgbm.dir/src/c_api.cpp.o -c /Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/src/c_api.cpp

2. Compilation command after commit 9c81ec6 introducing object library.

/Applications/Xcode_12.4.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -DEIGEN_MPL2_ONLY -DLGB_R_BUILD -DMM_MALLOC -DMM_PREFETCH -DUSE_SOCKET -I/Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/external_libs/eigen -I/Library/Frameworks/R.framework/Versions/3.6/Resources/include -I/Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/include -Xclang -fopenmp -std=c++11 -pthread -Wextra -Wall -Wno-ignored-attributes -Wno-unknown-pragmas -Wno-return-type -O3 -fPIC -funroll-loops -isysroot /Applications/Xcode_12.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.1.sdk -mmacosx-version-min=10.15 -MD -MT CMakeFiles/lightgbm_capi_objs.dir/src/c_api.cpp.o -MF CMakeFiles/lightgbm_capi_objs.dir/src/c_api.cpp.o.d -o CMakeFiles/lightgbm_capi_objs.dir/src/c_api.cpp.o -c /Users/runner/work/LightGBM/LightGBM/lightgbm.Rcheck/00_pkg_src/lightgbm/src/src/c_api.cpp

There are two more options before this PR:

1. -D_lightgbm_EXPORTS
   • Not sure if this is necessary.
2. -isystem /usr/local/include
   • This is necessary because we need to use openmp installed by homebrew.

Commit 33385b1 add those two options and fixes CI. But I can't figure out how those two options come into the compilation command.

I figure out why I can't reproduce the problem in conda environment. The environment is created with following command:

xcode-select --intsall
conda create -n r3.6 r-base=3.6.1
conda activate r3.6
conda install r-data.table r-jsonlite r-Matrix r-R6 r-testthat r-processx

The conda environment contains llvm-openmp and clang package and it indeed has the omp.h in environment's include directory. conda activate r3.6 will set CXXFLAGS to include that directory. Thus export CXX=/usr/bin/clang++ to use AppleClang can still successfully build the R-package.

[jameslamb]

Thanks for the investigation in #4489 (comment), @cyfdecyf . Do you need help looking into the errors on Azure DevOps? https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=10662&view=logs&j=ea56812e-e7ae-55d0-6abc-4a217857fa9f

-isystem /usr/local/include
This is necessary because we need to use openmp installed by homebrew.

Makes sense, I guess it's a similar problem to #4131, but for CMake-based builds.

[cyfdecyf]

CI failure for "Linux mpi_source" is caused by USE_DEBUG being set and thus -fPIC not included.

Commit ccafdab adds -fPIC when building shared library, not depending on USE_DEBUG option.

[cyfdecyf]

@jameslamb Thanks, I indeed need help for Windows bdist on Azure Devops https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=10668&view=logs&j=ea56812e-e7ae-55d0-6abc-4a217857fa9f&t=e61701f8-4e1b-5007-20cb-a000ab3587fb

[cyfdecyf]

I've tried the key building steps of windows bdist build on my local system with Visual Studio 2019.

• I have to change WindowsTargetPlatformVersion to 10 in windows/LightGBM.vcxproj, otherwise it complains that Windows 8.1 SDK not found. I'm not able to install that SDK in VS2019.
• python setup.py bdist_wheel --plat-name=win-amd64 --python-tag py3 works after the above change.
  • Passing --integrated-opencl to setup.py would result failure. Guess this is because I don't have opencl installed.

[cyfdecyf]

Printing out cmake output shows that windows bdist failed because boost is not added in include library. I'll try to figure out why.

[cyfdecyf]

I didn't notice there are target_compile_definitions, target_include_directories usage in CMakeLists.txt. They should be changed to define on the new lightgbm_objs object library target.

I'll check CMakeLists.txt later and see if we should change target_link_library to define on lightgbm_objs.

[cyfdecyf]

I'm defining all link dependencies, compile definitions to the object library object. This can remove duplicated definitions on two targets.

It's not easy to do this right in one shot, sorry for the CI noise.

[cyfdecyf]

@StrikerRUS My PR has conflict with master branch and can't run all CI tests. Resolving the conflit on github would introduce merge commit.

To keep a linear history, I made a rebase to current master and force push. Sorry for the noise.

[StrikerRUS]

@cyfdecyf

To keep a linear history, I made a rebase to current master and force push. Sorry for the noise.

No problem! We always "Squash and merge" PRs, so please don't worry about dirty commit history next time.

[cyfdecyf]

@StrikerRUS I made some corrections to the used of lightgbm_capi_objs. The special include directory handling for openmp build on macOS is no longer needed.

[StrikerRUS]

LGTM! Thank you so much for this contribution and for your really hard work with the debugging!

[jameslamb]

Works for me! Based on passing CI jobs and the review discussion, I'm comfortable with this PR.

Thanks for your hard work, and to @StrikerRUS for a very thorough review that required reading a lot of CMake documentation.

[shiyu1994]

@cyfdecyf @jameslamb @StrikerRUS Thank you all! Is this ready for merge?

[cyfdecyf]

@shiyu1994 It's ready for merge. Please notify me in case there are any problems.

[StrikerRUS]

@shiyu1994

Is this ready for merge?

This PR hasn't been merged yet for the same reason as in #4580 (comment). Do you think we should start merging PRs with large new features?

[jameslamb]

Do you think we should start merging PRs with large new features?

It has been 13 days since LightGBM 3.3.1 was released (#4715 (comment)), and one month since LightGBM 3.3.0 was released (#4633 (comment)).

In the comments linked from #4580 (comment), we discussed waiting two weeks.

I think it has been close enough to two weeks that it is ok to start merging larger PRs and working towards LightGBM 4.0.

[github-actions]

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.