You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@orc.apache.org by jcrist <gi...@git.apache.org> on 2017/11/01 22:25:51 UTC
[GitHub] orc pull request #185: ORC-261: [C++] Fix installation to include all header...
GitHub user jcrist opened a pull request:
https://github.com/apache/orc/pull/185
ORC-261: [C++] Fix installation to include all header files
Addresses [issue 261](https://issues.apache.org/jira/projects/ORC/issues/ORC-261).
- Fixes installation to include all necessary header files in `include/orc`
- Removes a few unnecessary includes
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jcrist/orc fix-install-includes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/orc/pull/185.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #185
----
commit 92200217641d645cd382f0cab5c8ffc33e62e5b7
Author: Jim Crist <ji...@gmail.com>
Date: 2017-11-01T22:20:00Z
Fix installation to include all header files
- Fixes installation to include all necessary header files in
`include/orc`
- Removes a few unnecessary includes
----
---
[GitHub] orc issue #185: ORC-261: [C++] Fix installation to include all header files
Posted by jcrist <gi...@git.apache.org>.
Github user jcrist commented on the issue:
https://github.com/apache/orc/pull/185
Note that this is the first time I've touched C++ code outside undergrad, so there's a decent chance I've done something wrong here. Everything seems to pass locally for me though.
---
[GitHub] orc issue #185: ORC-261: [C++] Fix installation to include all header files
Posted by omalley <gi...@git.apache.org>.
Github user omalley commented on the issue:
https://github.com/apache/orc/pull/185
+1 This looks good.
---
[GitHub] orc pull request #185: ORC-261: [C++] Fix installation to include all header...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/orc/pull/185
---
[GitHub] orc pull request #185: ORC-261: [C++] Fix installation to include all header...
Posted by jcrist <gi...@git.apache.org>.
Github user jcrist commented on a diff in the pull request:
https://github.com/apache/orc/pull/185#discussion_r148402662
--- Diff: tools/src/CMakeLists.txt ---
@@ -10,10 +10,17 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+# TODO:
+# - orc-metadata relies on the protobuf routines, meaning protobuf and
+# binary_dir/c++/src still need to be included
+# - timezone-dump relies on non-public timezone routines. I *think* this
+# executable can just be removed, as it looks like it was written for testing
+# alone.
--- End diff --
If these issues can be resolved, then the `include_directories` below can just be:
```
include_directories (
${PROJECT_SOURCE_DIR}/c++/include
${PROJECT_BINARY_DIR}/c++/include
)
```
meaning the executables use the same interface a user of this library would get. Whether that means the protobuf headers are included in the build, or the interface is expanded to make `orc-metadata` not rely on the protobuf headers directly I'm not sure.
Either way this probably doesn't need to be done now, it's just a potential hygiene thing.
---