-
Notifications
You must be signed in to change notification settings - Fork 69
Update cmake instructions in README #24
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
Conversation
| # under the License. | ||
|
|
||
| build/ | ||
| install/ |
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.
Why this? Is this generated by some IDE?
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.
No, since we specify CMAKE_INSTALL_PREFIX in cmake, I think someone might specify CMAKE_INSTALL_PREFIX=install in the repo directory as well.
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.
No, since we specify
CMAKE_INSTALL_PREFIXin cmake, I think someone might specifyCMAKE_INSTALL_PREFIX=installin the repo directory as well.
I doubt anyone would do this, if you really need to install in src tree, you can specify CMAKE_INSTALL_PREFIX=/build/install.
| ```bash | ||
| cd iceberg-cpp | ||
| cmake -S . -B build -DCMAKE_INSTALL_PREFIX=/path/to/install -DICEBERG_BUILD_STATIC=ON -DICEBERG_BUILD_SHARED=ON | ||
| cmake -S . -B build -DCMAKE_INSTALL_PREFIX=/path/to/install -DICEBERG_BUILD_STATIC=ON -DICEBERG_BUILD_SHARED=ON -DICEBERG_BUILD_TESTS=OFF -DICEBERG_ARROW=OFF |
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.
Maybe these options should be documented somewhere but not specified in the same example command?
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.
Agreed. An official site for iceberg-cpp is more suitable for these documentations.
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.
Should we open an issue to choose a documentation/site generator?
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 would be good if someone with expertise could lead this :)
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.
This PR is to update cmake to build Core Libraries, and I think we don't need to build Arrow for the core libraries, right?
Build
Core Librariesw/oArrow, as there is another section about buildingIceberg Arrow Libraryexplicitly.