-
-
Notifications
You must be signed in to change notification settings - Fork 8.7k
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
Use configure_file() to configure version only #4974
Conversation
@trivialfis If the goal is to avoid polluting the include directory, can we apply something like dmlc/dmlc-core#551? |
We should formally enforce this, rather than making it an unwritten convention. Perhaps create a separate config step just for the version ( Let me think of something and get back to you. |
@hcho3 Good point. |
Let me try incorporating ideas from dmlc/dmlc-core#551 and dmlc/dmlc-core#566 and get back to you. |
@hcho3 I removed the I also added a |
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 see your point about the "include only what you need" principle. At best, you'd get an error like "XGBOOST_VER_MAJOR
symbol not found" and you'll be reminded to include xgboost/version_config.h
.
Note: Should we need to use |
@hcho3 Agreed. until we can remove those makefiles, your method works better. |
Will merge once tests pass. |
Codecov Report
@@ Coverage Diff @@
## master #4974 +/- ##
=======================================
Coverage 71.82% 71.82%
=======================================
Files 11 11
Lines 2300 2300
=======================================
Hits 1652 1652
Misses 648 648 Continue to review full report at Codecov.
|
@hcho3 Thanks for the help. ;-) |
@hcho3 This keeps most of your clean up, but added back the intrin detection code in order to avoid writing
build_config.h
in include directory. After this, only version upgrade can change that file.