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

change include file path #130

Closed
wants to merge 1 commit into from
Closed

change include file path #130

wants to merge 1 commit into from

Conversation

CodingCat
Copy link
Member

not sure why we had this change...but I found it broke the original command mvn package running in mac

@trivialfis @chenqin

@chenqin
Copy link
Contributor

chenqin commented Dec 1, 2019

LGTM, I thought we had rabit import path issue with cmake file.

@trivialfis
Copy link
Member

trivialfis commented Dec 1, 2019

I'm not sure about the relative paths in public headers. We have -Idmlc-core/include, some headers in there can sneak into the translation unit replacing the one in rabit (which can lead to obscure bugs). While using rabit/serialisation.h provide some weak guarantees that no such error can occur silently.

For the reason why this could happen, include path searching starts with current file, but not limited to it. If the compiler fails to find one in relative path to current file, it will start again from paths specified by -I flag. (-I is a short hand compiler argument saying include this directory, more restricted setting would distinguish -isystem and -I, but should not be related to this PR) I would like to resolve your build issue first, it may indicates errors described above is actually happening in our code base.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

Could you please provide the error message you encountered so I can take a look?

@CodingCat
Copy link
Member Author

Could you please provide the error message you encountered so I can take a look?

it actually simply cannot find files like rabit/serializable.h

@hcho3
Copy link
Contributor

hcho3 commented Nov 5, 2020

Recent developments of Rabit have been moved into the XGBoost repository. See discussion in dmlc/xgboost#5995.

@hcho3 hcho3 closed this Nov 5, 2020
@hcho3 hcho3 deleted the change_include_path branch November 5, 2020 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants