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

[ci] use lowercase library names in linker flags #5870

Merged
merged 2 commits into from
May 6, 2023

Conversation

characat0
Copy link
Contributor

This change should improve compatibility and consistency with other projects and build tools, and prevent any potential issues arising from the inconsistent capitalization of the library names.

Changed casing of link libraries:
Ws2_32 -> ws2_32
IPHLPAPI -> iphlpapi
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 taking the time to contribute to LightGBM!

Can you give some specific examples of "potential issues arising from the inconsistent capitalization of the library names" to help us understand this proposed change?

@characat0
Copy link
Contributor Author

I was trying to create libraries for Julia using BinaryBuilder.jl, during compilation I found the following errors for windows:

/opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/7.1.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lWs2_32
/opt/x86_64-w64-mingw32/bin/../lib/gcc/x86_64-w64-mingw32/7.1.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find -lIPHLPAPI

when refering to the documentation of the builders, I found that the libraries were in lowercase for linux, and since windows is case insensitive, I think this change would allow more projects to build LightGBM without making downstream patches.

@characat0 characat0 requested a review from jameslamb May 6, 2023 19:12
@jameslamb jameslamb changed the title [ci] capitalization of Windows library names [ci] use lowercase library names in linker flags May 6, 2023
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.

Works for me! We don't use MinGW to cross-compile Windows artifacts on Linux (we just directly compile on Windows), so haven't run into the issues you've described.

But we've been using all-lowercase library names in the same linker flags for CMake-driven builds of the R package on Windows for a few years now, without issue:

PKG_LIBS = \
${SHLIB_OPENMP_CXXFLAGS} \
${SHLIB_PTHREAD_FLAGS} \
-lws2_32 \
-lIphlpapi

So given that + all the passing CI jobs, I'm confident this change won't cause issues.

Thanks for the contribution!

@jameslamb
Copy link
Collaborator

Oh wait actually I just realized there's a capital I there. Could you please update that place too?

@jameslamb jameslamb self-requested a review May 6, 2023 21:53
@characat0 characat0 requested a review from jmoralez as a code owner May 6, 2023 21:55
@characat0
Copy link
Contributor Author

@characat0 please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

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 so much!

@characat0
Copy link
Contributor Author

It's great to start contributing to the libraries I use
Thank you for your guidance

@jameslamb
Copy link
Collaborator

of course! We'd love to have more of your help here in LightGBM if you have time and interest 😁

@jameslamb jameslamb merged commit 9078696 into microsoft:master May 6, 2023
@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 15, 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.

2 participants