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

Moved definitions into a config #3479

Closed
wants to merge 1 commit into from
Closed

Moved definitions into a config #3479

wants to merge 1 commit into from

Conversation

KOLANICH
Copy link
Contributor

No description provided.

@hcho3
Copy link
Collaborator

hcho3 commented Jul 17, 2018

Could you provide a rationale for this change?

@tqchen
Copy link
Member

tqchen commented Jul 17, 2018

Let us still keep the old one, as the config infra is dependent on cmake, and may not be useful for amagamaltion

@KOLANICH
Copy link
Contributor Author

KOLANICH commented Jul 17, 2018

Could you provide a rationale for this change?

Yes. It stores configuration parameters with the library, which are a link between binaries and headers. So
1 no logic determining these parameters and inserting them into required for third-party libs. They are precomputed during compilation process and shipped with headers.
2 these parameters can be used by third-party code.

In fact that's why creating a config was chosen over add_definitions in dmlc/dmlc-core#432 .

I also wanna note that this approach is used by large projects using CMake, like OpenCV and libcurl.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 23, 2018

@KOLANICH Unlike dmlc/dmlc-core#432, where we enabled MinGW/Cygwin support, this pull request doesn't appear to have tangible benefits. I'd definitely adopt config approach for new projects that exclusively use CMake, but XGBoost will have to keep legacy build system for uses cases such as amalgamation.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 23, 2018

@KOLANICH Actually, we could do what we did dmlc/dmlc-core#432 (supply a default built build_config.h) to support amalgamation to make this pull request viable. Now the question would be: what would be tangible benefits? For example, does the new approach makes life easier for 3rd-party developers who embed XGBoost in their applications?

@trivialfis
Copy link
Member

@hcho3 Other than amagamaltion, is there other reason for keeping these makefiles?

@hcho3
Copy link
Collaborator

hcho3 commented Oct 22, 2018

@trivialfis Makefile exists mainly for legacy reasons. If I were to start a new project, I would use CMake exclusively.

@trivialfis
Copy link
Member

@hcho3 Thanks. But I got curious what are legacy reasons? Eventually I want to remove these makefiles. Amagamaltion can be handled by CMake, maybe I can solve other issues step by step. :)

@hcho3
Copy link
Collaborator

hcho3 commented Oct 22, 2018

Amagamaltion can be handled by CMake

That's great to hear.

@tqchen Any reason why we should still keep Makefile? Do we support systems without CMake?

@hcho3
Copy link
Collaborator

hcho3 commented Oct 22, 2018

@trivialfis CMake was added in July 2016, whereas Makefile was around since the beginning of the project (2014). So better to say that "it's the way things have been done"

@trivialfis
Copy link
Member

@hcho3 Great, I will try to think about this.

@trivialfis
Copy link
Member

Closing for now. Without getting rid of makefiles this won't be possible. And I don't want to set the output path of configured file inside source tree. Sorry for the long stall.

@trivialfis trivialfis closed this Jan 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Apr 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants