Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Add option for external C++ development toolchain, use C++11, configure libparquet library / header installation #13

Closed
wants to merge 14 commits into from

Conversation

wesm
Copy link
Member

@wesm wesm commented Dec 24, 2015

This PR makes no functional changes to the codebase, but is more a reorganization for using parquet-cpp as a building block for other native code projects. After this I'm going to start working on functional unit tests and working toward a complete reader / writer implementation.

Notably this patch removes the thirdparty packages and will rely either on cloudera/native-toolchain for development / linking or system-level dynamic libraries. You can also perform out-of-source builds.

I have not tried getting Travis CI to work with the native toolchain; I may enlist the help of @grundprinzip on this portion.

@wesm
Copy link
Member Author

wesm commented Dec 24, 2015

Can someone let me know what are the appropriate copyright headers to use in these files? In copy-pasting from other Apache / Apache-licensed Cloudera C++ projects like Impala and Kudu I've transferred a few Cloudera copyright headers

@wesm wesm mentioned this pull request Dec 24, 2015
@wesm wesm changed the title Migrate to external C++ development toolchain, use C++11, configurate libparquet library / header installation PARQUET-416: Migrate to external C++ development toolchain, use C++11, configurate libparquet library / header installation Dec 24, 2015
@wesm wesm changed the title PARQUET-416: Migrate to external C++ development toolchain, use C++11, configurate libparquet library / header installation PARQUET-416: Migrate to external C++ development toolchain, use C++11, configure libparquet library / header installation Dec 24, 2015
@spena
Copy link

spena commented Dec 28, 2015

@wesm I think the license header should contain only the Apache license information. You may want to remove the Cloudera copyright as Parquet is now part of the Apache community. A header like this may be pasted on those cpp or cmake files:

/**

  • Licensed to the Apache Software Foundation (ASF) under one
  • or more contributor license agreements. See the NOTICE file
  • distributed with this work for additional information
  • regarding copyright ownership. The ASF licenses this file
  • to you under the Apache License, Version 2.0 (the
  • "License"); you may not use this file except in compliance
  • with the License. You may obtain a copy of the License at
    *
  • http://www.apache.org/licenses/LICENSE-2.0
  • Unless required by applicable law or agreed to in writing,
  • software distributed under the License is distributed on an
  • "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
  • KIND, either express or implied. See the License for the
  • specific language governing permissions and limitations
  • under the License.
    */

@wesm
Copy link
Member Author

wesm commented Dec 28, 2015

Thanks -- since this codebase never got "shipped" in the past this has not been an issue. I can fix the copyright headers in a follow up patch so avoid adding any more noise to this patch if that is OK.

@wesm wesm force-pushed the libparquet-dev-environment branch from d5f2d6f to 3268411 Compare January 5, 2016 02:12
@asandryh
Copy link
Contributor

asandryh commented Jan 5, 2016

Hi Wes,
Can you please clarify the motivation for this code reorganization? I haven't had problems building parquet-cpp against system-installed or manually built libraries. With your changes, the build fails. Note that your pull request removes /thirdparty directory, so you certainly should update build instructions.
Overall, it is not necessary at all that building parquet-cpp should rely on cloudera/native-toolchain. This is a stand-alone library, and linking in external libraries should be performed via user-specified CMake options.
Thank you.

@wesm
Copy link
Member Author

wesm commented Jan 5, 2016

@asandryh I completely agree — per #10 and PARQUET-267 I am going to ensure that building with system-installed libraries or indicating 3rd party dependencies via cmake options is the default and simplest option. The motivation in adding an external toolchain option was to facilitate development and linking with other C++ libraries sharing a common portable Linux toolchain.

@asandryh
Copy link
Contributor

asandryh commented Jan 5, 2016

@wesm Thank you for the clarification. It looks like @kalaxy in #10 has exactly the same concerns as I.
Your proposed refactoring is very reasonable. My main concern with this pull request is that, with your current modifications to CMake config, the library does not build with user-specified libraries.

@wesm wesm changed the title PARQUET-416: Migrate to external C++ development toolchain, use C++11, configure libparquet library / header installation PARQUET-416: Add option for external C++ development toolchain, use C++11, configure libparquet library / header installation Jan 5, 2016
@nongli
Copy link
Contributor

nongli commented Jan 6, 2016

@wesm can you split this patch in two? I think the one that rearranges the source structure, headers, #define cleanups etc are not controversial and can go in. The build step needs more discussion.

@nongli
Copy link
Contributor

nongli commented Jan 6, 2016

@wesm can you participate in the discussion here
https://issues.apache.org/jira/browse/PARQUET-267

@wesm
Copy link
Member Author

wesm commented Jan 6, 2016

@nongli no problem, I'll spend some time this afternoon and open a new build environment patch per discussion in PARQUET-267.

@wesm
Copy link
Member Author

wesm commented Jan 7, 2016

Closing in favor of #14 and follow up patch re: more flexible build environment configuration.

@wesm wesm closed this Jan 7, 2016
@wesm wesm changed the title PARQUET-416: Add option for external C++ development toolchain, use C++11, configure libparquet library / header installation Add option for external C++ development toolchain, use C++11, configure libparquet library / header installation Jan 7, 2016
asfgit pushed a commit that referenced this pull request Jan 8, 2016
…on targets

Reorganize code into a top level src/parquet directly, add a libparquet shared library, and add install targets for libparquet and its header files. Add cpplint script and `make lint` target for code linting.

Replaces earlier PR #13

Author: Wes McKinney <wes@cloudera.com>

Closes #14 from wesm/libparquet-library and squashes the following commits:

2e356fd [Wes McKinney] PARQUET-416: Compile with C++11 and replace usages of boost::shared_ptr with std::shared_ptr and other C++11 fixes. Reorganize code into a top level src/parquet directly, add a libparquet shared library, and add install targets for libparquet and its header files. Add cpplint script and `make lint` target for code linting.
@wesm wesm deleted the libparquet-dev-environment branch March 1, 2016 00:30
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants