-
Notifications
You must be signed in to change notification settings - Fork 256
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
[Version Bump Configuration]add configure version in hpp file #621
Conversation
Code Coverage Report
|
CMakeLists.txt
Outdated
|
||
configure_file( | ||
${PROJECT_SOURCE_DIR}/include/ylt/ylt.hpp.in | ||
${PROJECT_SOURCE_DIR}/include/ylt/ylt.hpp |
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.
It might be better to put the genereated files in the build folder.
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.
Any example for generated file?
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.
use PROJECT_BINARY_DIR
and adjust include path too
configure_file(
${PROJECT_SOURCE_DIR}/include/ylt/ylt.hpp.in
${PROJECT_BINARY_DIR}/include/ylt/ylt.hpp
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.
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.
use
PROJECT_BINARY_DIR
and adjust include path tooconfigure_file( ${PROJECT_SOURCE_DIR}/include/ylt/ylt.hpp.in ${PROJECT_BINARY_DIR}/include/ylt/ylt.hpp
Put it in binray dir will make users confused since they can not fond any clues about generated files.
Actullay it's SOURCE FILES definitely.
I think we can reference asio:https://github.com/chriskohlhoff/asio/blob/master/asio/include/asio/version.hpp It's more easier to maintain, only upgrade the version file when release a version. |
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.
When user just include the header without build, this maybe failed. I think we can just add the macro by manually without cmake.
Did you mean rename them in version.h |
Code Coverage Report
|
Code Coverage Report
|
It's easy to do this, but branch name and commmit id might not be generated dynamically? |
Code Coverage Report
|
If use cmake to generate the version file, when the user copy ylt headers and include the header and include version.hpp will cause error, because the file is not exist. I think just ceate a version.hpp to the include directory is ok, don't need cmake. |
1d25cb7
to
19325d1
Compare
Code Coverage Report
|
Why
Users can get ylt version from header file and specific package with git hash might help us to locate detail branch and commit id.
What is changing
Example