orc-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From jcrist <...@git.apache.org>
Subject [GitHub] orc pull request #185: ORC-261: [C++] Fix installation to include all header...
Date Wed, 01 Nov 2017 22:31:31 GMT
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.


---

Mime
View raw message