Skip to content
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

doc: Add description about enable_apache_arrow for configure #1383

Merged

Conversation

HashidaTKS
Copy link
Contributor

@HashidaTKS HashidaTKS commented Aug 10, 2022

#1382 (comment)

  • Add description for configure
  • Add description for PKG_CONFIG_PATH

I have done the following tasks in another PR #1403 because the scope is slightly different

  • Add description for Windows(CMake)
  • Add description for CMAKE_PREFIX_PATH

@HashidaTKS
Copy link
Contributor Author

@kou

Would you review of the changes so far?
I have added descriptions about enable-apache-arrow , disable-apache-arrow and PKG_CONFIG_PATH for configure.

@yoshimotoyuk

Would you review English of this patch when you have time?

doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
doc/source/install/others.rst Outdated Show resolved Hide resolved
@HashidaTKS HashidaTKS force-pushed the doc_add_enable_apache_arrow_to_configure branch from 4db3101 to deaf748 Compare September 1, 2022 05:26
@HashidaTKS
Copy link
Contributor Author

I have done the following tasks in another PR #1403 because the scope is slightly different

  • Add description for Windows(CMake)
  • Add description for CMAKE_PREFIX_PATH

@HashidaTKS HashidaTKS marked this pull request as ready for review September 5, 2022 01:45
@HashidaTKS
Copy link
Contributor Author

@kou, @yoshimotoyuk

Thank you for the review.
I have addressed the feedback comments.


.. note::

If you install Apache Arrow manually, you need to the :ref:`install-others-configure-pkg-config-path` option.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
If you install Apache Arrow manually, you need to the :ref:`install-others-configure-pkg-config-path` option.
If you install Apache Arrow into non-standard directory, you need to specify the :ref:`install-others-configure-pkg-config-path` option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

doc/source/install/others.rst Outdated Show resolved Hide resolved
@HashidaTKS
Copy link
Contributor Author

@kou

Would you please re-review this?

You can use ``PKG_CONFIG_PATH`` as an environment variable, but we recommend
to use it as a ``configure`` parameter because of the following reason.

``configure.ac`` generates ``configure``. And when ``make`` detects that
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

configure is generated from configure.ac by GNU Autotools.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.


.. code-block:: console

% ./configure PKG_CONFIG_PATH=/tmp/local/lib/pkgconfig/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
% ./configure PKG_CONFIG_PATH=/tmp/local/lib/pkgconfig/
$ ./configure PKG_CONFIG_PATH=/tmp/local/lib/pkgconfig/

Does Pygments accept % as prompt? If yes, I'm OK with %.

FYI: Rouge that is used by Jekyll accepts only $ for prompt.

Copy link
Contributor Author

@HashidaTKS HashidaTKS Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe accepted.
I couldn't find any documents about whether we can use % but I didn't get any warning about it and it seems to be displayed correctly.

image

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HashidaTKS
Copy link
Contributor Author

HashidaTKS commented Sep 8, 2022

@kou

Would you please re-review this?

@kou kou merged commit 0da9aeb into groonga:master Sep 12, 2022
kou pushed a commit that referenced this pull request Sep 13, 2022
I have added descriptions about the whole build procedure with CMake.

Related: #1383

Co-authored-by: Yukiko Yoshimoto <86996282+yoshimotoyuk@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants