-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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] dowgrade gcc version for Linux on Azure #1718
Conversation
GPU test fails. I suppose that the reason is in these lines:
While installing OpenCL libraries @guolinke Since we don't provide any artifacts from GPU test publicly, what do you think about the following?
|
@StrikerRUS yeah, that is okay |
.ci/setup.sh
Outdated
if [[ $AZURE == "true" ]] && [[ $COMPILER == "clang" ]]; then | ||
update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-6.0 100 | ||
update-alternatives --install /usr/bin/clang clang /usr/bin/clang-6.0 100 | ||
if [[ $AZURE == "true" ]]; then | ||
sudo apt-get update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be we should update for travis as well, since line 48 will install ocl-icd-opencl-dev
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested without sudo apt-get update
at Travis - worked well. But if it doesn't significantly enlarge the test time, lets leave this.
@StrikerRUS
So i add some fixes . |
include/LightGBM/meta.h
Outdated
@@ -62,6 +62,10 @@ typedef void(*AllgatherFunction)(char* input, comm_size_t input_size, const comm | |||
#define __func__ __FUNCTION__ | |||
#endif | |||
|
|||
#ifdef GCC | |||
__asm__(".symver lgamma, lgamma@GLIBC_2.2.5"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a better solution is implementing a lgamma
by ourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, why is this symver needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgamma
will introduce the glibc2.23 dependency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds a bit odd. I've checked the glibc changelog and it seems lgamma
have been there for quite a while.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think lgamma have a new implementation in glibc2.23. and result in this new dependency.
CMakeLists.txt
Outdated
@@ -17,6 +17,10 @@ if(APPLE) | |||
OPTION(APPLE_OUTPUT_DYLIB "Output dylib shared library" OFF) | |||
endif() | |||
|
|||
if((CMAKE_CXX_COMPILER_ID STREQUAL "GNU") AND (UNIX) AND (NOT APPLE)) | |||
ADD_DEFINITIONS(-DGCC) | |||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to move this somewhere lower to other definitions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I didn't understand it, can you describe more details ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean, move these lines lower in the file. For example, somewhere around here
https://github.com/Microsoft/LightGBM/blob/606eeefeb8e1f217a0f91fa0f11137125942b549/CMakeLists.txt#L66-L68
or here
https://github.com/Microsoft/LightGBM/blob/606eeefeb8e1f217a0f91fa0f11137125942b549/CMakeLists.txt#L118-L119
where other ADD_DEFINITIONS
are set.
@StrikerRUS what do you think about the current fix for |
@guolinke To be honest, I'm not familiar with SymbolVersioning, but I think it's rather hacky solution and intuitively I can't say that I like it. I'm afraid that it will cause some troubles. I'd better prefer this your proposal:
Can we just copy-paste this function from the sources? |
@StrikerRUS I just notice we don't need the lgamma. The new fix is pushed.
|
What is the glibc version used by GCC 4.8? Could you mention it in the PR description? |
Wow, it's great! This fix is for sure much better than symbol versioning. 😃 |
@guolinke I've added |
|
Should we expect a release following this PR? |
yeah, refer to #1727 |
Refer to #1708.