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

cmake: add missing lib_lightgbm_swig.so to lightgbmlib.jar on linux #6515

Merged
merged 3 commits into from
Jul 4, 2024

Conversation

shuttie
Copy link
Contributor

@shuttie shuttie commented Jul 2, 2024

A PR [cmake] [swig] use CMake's built-in file-copying mechanisms instead of 'cp' #6259 introduced a small build regression:

  • before: for Linux, both lib_lightgbm.so and lib_lightgbm_swig.so were included into the JAR
  • after: only lib_lightgbm.so is part of the JAR.

Lack of the SWIG wrapper makes it impossible to link to the library from the JVM. Mac and Windows builds do include the SWIG wrapper properly and are not affected by this regression.

This PR adds back the lib_lightgbm_swig.so library to the JAR build process on Linux. I'm maintaining the lightgbm4j library and find it very convenient to re-use your build artifacts :)

@shuttie
Copy link
Contributor Author

shuttie commented Jul 3, 2024

The failure on Microsoft.LightGBM/QEMU_multiarch bdist CI test seem to be unrelated to the change.

@jameslamb
Copy link
Collaborator

You're right, we have a project-wide blocking CI issue right now: #6509.

We'll come back and review this once we've fixed that. Thanks so much for taking the time to contribute, and sorry about breaking that in #6259 😭

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks very much for the help!

The SWIG builds here haven't been very actively developed (https://github.com/microsoft/LightGBM/commits/master/swig), and the only tests we run on them is just "does compilation succeed".

LightGBM/.ci/test.sh

Lines 83 to 95 in f811c82

if [[ $TASK == "swig" ]]; then
cmake -B build -S . -DUSE_SWIG=ON
cmake --build build -j4 || exit 1
if [[ $OS_NAME == "linux" ]] && [[ $COMPILER == "gcc" ]]; then
objdump -T ./lib_lightgbm.so > ./objdump.log || exit 1
objdump -T ./lib_lightgbm_swig.so >> ./objdump.log || exit 1
python ./helpers/check_dynamic_dependencies.py ./objdump.log || exit 1
fi
if [[ $PRODUCES_ARTIFACTS == "true" ]]; then
cp ./build/lightgbmlib.jar $BUILD_ARTIFACTSTAGINGDIRECTORY/lightgbmlib_$OS_NAME.jar
fi
exit 0
fi

If you have ideas for improving that, we'd welcome the help! For example, I think a some Java unit tests (even just very minimal ones that train a small regression model and maybe generate some predictions) would have caught this bug, and would catch future bugs before they make it to a release. We don't really have much Java experience on the maintainer team, so we'd welcome the help adding something like that (in later PRs) if it interests you.

@jameslamb jameslamb merged commit 83df24d into microsoft:master Jul 4, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants