-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
GH-41783: [C++] Make git-dependent definitions internal #41781
Conversation
Thanks for opening a pull request! If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format?
or
In the case of PARQUET issues on JIRA the title also supports:
See also: |
ea2b913
to
1f2709b
Compare
@github-actions crossbow submit -g cpp |
Revision: 1f2709b Submitted crossbow builds: ursacomputing/crossbow @ actions-500c735934 |
|
1f2709b
to
bf026f7
Compare
Exposing the ARROW_GIT_ID and ARROW_GIT_DESCRIPTION preprocessor variables in our public headers tends to make incremental builds less efficient, since those values change very often during development. Also, these values don't need to be preprocessor variables since they're just strings: you can't write useful `#if` directives with them. Instead, they can be inspected using `GetBuildInfo()`.
bf026f7
to
2d21b2e
Compare
@kou Does this look ok to you? |
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.
+1
Ah, good point. I missed it.
After merging your PR, Conbench analyzed the 7 benchmarking runs that have been run so far on merge-commit 9185d7d. There weren't enough matching historic benchmark results to make a call on whether there were regressions. The full Conbench report has more details. |
…#41781) ### Rationale for this change Exposing the ARROW_GIT_ID and ARROW_GIT_DESCRIPTION preprocessor variables in our public headers tends to make incremental builds less efficient, since those values change very often during development. Also, these values don't need to be preprocessor variables since they're just strings: you can't write useful `#if` directives with them. Instead, they can be inspected using `GetBuildInfo()`. ### Are these changes tested? By existing builds and tests. ### Are there any user-facing changes? Use cases depending on these preprocessor variables, which is unlikely, may break. They can be fixed by calling `arrow::GetBuildInfo()` instead. * GitHub Issue: apache#41783 Authored-by: Antoine Pitrou <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
Rationale for this change
Exposing the ARROW_GIT_ID and ARROW_GIT_DESCRIPTION preprocessor variables in our public headers tends to make incremental builds less efficient, since those values change very often during development.
Also, these values don't need to be preprocessor variables since they're just strings: you can't write useful
#if
directives with them. Instead, they can be inspected usingGetBuildInfo()
.Are these changes tested?
By existing builds and tests.
Are there any user-facing changes?
Use cases depending on these preprocessor variables, which is unlikely, may break. They can be fixed by calling
arrow::GetBuildInfo()
instead.