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

[swig] switch to SWIG_ADD_LIBRARY() in CMakeLists.txt (fixes #5586) #5603

Merged
merged 3 commits into from
Nov 27, 2022

Conversation

shogohida
Copy link
Contributor

Signed-off-by: Shogo Hida [email protected]

Fixes #5586

Signed-off-by: Shogo Hida <[email protected]>
@shogohida shogohida marked this pull request as ready for review November 24, 2022 19:30
@jameslamb jameslamb changed the title fix: Fix CMakeLists.txt [swig] switch to SWIG_ADD_LIBRARY() in CMakeLists.txt Nov 25, 2022
@jameslamb jameslamb changed the title [swig] switch to SWIG_ADD_LIBRARY() in CMakeLists.txt [swig] switch to SWIG_ADD_LIBRARY() in CMakeLists.txt (fixes #5586) Nov 25, 2022
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 for this! I've modified the PR title so that it's more informative. PR titles become changelog items in this project's release notes (see https://github.com/microsoft/LightGBM/releases/tag/v3.3.3 for example), so we try be more specific than "fix CMakeLists.txt".

Please also fix the failing CI jobs, e.g. this one: https://dev.azure.com/lightgbm-ci/lightgbm-ci/_build/results?buildId=13909&view=logs&j=535e9539-0564-5d34-2431-da2644b83c50&t=7cfc64df-bff4-5eef-5deb-8ea58a924da8.

CMake Error at /usr/local/share/cmake-3.23/Modules/UseSWIG.cmake:752 (message):
SWIG_ADD_LIBRARY: java;swig/lightgbmlib.i: unexpected arguments
Call Stack (most recent call first):
CMakeLists.txt:460 (swig_add_library)

CMakeLists.txt Outdated Show resolved Hide resolved
@shogohida shogohida requested review from jameslamb and removed request for shiyu1994, guolinke and StrikerRUS November 26, 2022 04:42
@shogohida
Copy link
Contributor Author

@jameslamb
I fixed the points you mentioned so please review again!

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.

Looks great, thanks very much for helping to resolve this deprecation warning so new releases of CMake won't disrupt LightGBM's JVM users. We hope you'll consider more contributions in the future.

@jameslamb jameslamb merged commit 8f9133b into microsoft:master Nov 27, 2022
@shogohida shogohida deleted the switch-to-swig-add-library branch November 27, 2022 03:44
@shogohida
Copy link
Contributor Author

Thanks for reviewing my PR! Hope to contribute more in the future!

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[swig] switch to SWIG_ADD_LIBRARY() in CMakeLists.txt
2 participants